lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Erick Erickson <erickerick...@gmail.com>
Subject Re: [jira] Commented: (LUCENE-2285) Code cleanup from all sorts of (trivial) warnings
Date Thu, 25 Feb 2010 18:36:39 GMT
I don't have my heart set on keeping the deprecation, so taking it off works
for me. I'd also agree that we need a concerted effort to either completely
convert or we should leave it un-deprecated so feel free.

Let's move the junit4 stuff off to another discussion.

Erick


On Thu, Feb 25, 2010 at 1:27 PM, Shai Erera <serera@gmail.com> wrote:

> Erik, I'm totally with you on JUnit 4. I think the @Test annotation is
> really not a big deal (it's actually very easy to migrate all the current
> tests to JUnit 4 with the added import using some script. Even manually it
> should be such a big deal.
>
> @Ignore is a perfect other advantage of JUnit4. I've found some tests which
> were prefixed with _, i.e. _testXYZ just to disable them. Nobody knows about
> it until he looks at the code (and pays attention). @Ignore would have been
> better.
>
> And there are lots of other advantages, like the @Before and @After (not
> only class). Another problem I've found in the tests is that not all
> extended LuceneTestCase, and usually their setUp and tearDown
> implementations were wrong - not calling super first/last. When I moved them
> to extend LuceneTestCase they broke (I fixed them, don't worry). However,
> that could never happen if the super's methods were tagged w/ @Before/After,
> because JUnit would take care running them before/after their sub-classes'
> @Before/After. So that's another win for JUnit4.
>
> And of course the @Before/AfterClass are really great !
>
> So all in all, I'm a big fan of JUnit4, and if the discussion will start
> again, I'll pay more attention to it and participate (I admit I didn't
> follow it before). As long as it happens on the list and not on some IRC
> channel (!?!?).
>
> But like Uwe said, that's slightly unrelated to that issue. Because that
> deprecation alone produced > 500 warnings (probably even much more), I
> un-deprecated it, and when we make a decision one way or the other, we
> should simply remove it (in case that's the decision). Until then, let's get
> rid of the unnecessary noise, agree?
>
> Shai
>
>
> On Thu, Feb 25, 2010 at 7:15 PM, Uwe Schindler <uwe@thetaphi.de> wrote:
>
>>  This discussion is out oft he scope of this issue. We can start the
>> flamewar again. In IRC we came to the conculsion, that our primary intent is
>> to make the test runs faster, which we achieved by patching lots of tests to
>> not change static defaults and so be able to run all tests in the same JVM
>> without forking. More speed improvements can be done by moving read-only
>> index creation for search tests into static @BeforeClass and setting
>> IndexReaders/-Searchers to NULL in @AfterClass to allow GC of static fields
>> holding RAMDirectory and so on.
>>
>>
>>
>> The @Test annotation lead to more confusion and errors at our delevopers.
>> E.g. we had a test merged back from 3.0 (without Junit4) to trunk or even
>> new tests were added, but nobody added @Test to it, leading to the fact that
>> the test were never run. So the most important change to LuceneTestCaseJ4
>> would be to emulate the old test* method names as if they have @Test. By
>> that you could still disable them as mentioned, but it would reduce the
>> burden of these dumb import statements and useless annotations.
>>
>>
>>
>> By the way, why does LuceneTestCaseJ4 extend TestWatchman and also a
>> instance field extends that class? I do not understand the whole magic
>> behind, this is totally confusing to me – annotating a field that is never
>> used in code by an annotation is stupid and looks totally incorrect (I mean
>> the field holding the TestWatchman-subclass). - This is another thing why I
>> am against the migration of our already proven tests.
>>
>>
>>
>> Because of that we don’t want to deprecate LuceneTestCase and instead only
>> transform new tests and such needing @BeforeClass/@AfterClass for more speed
>> to the new API.
>>
>>
>>
>> -----
>>
>> Uwe Schindler
>>
>> H.-H.-Meier-Allee 63, D-28213 Bremen
>>
>> http://www.thetaphi.de
>>
>> eMail: uwe@thetaphi.de
>>
>>
>>
>> *From:* Erick Erickson [mailto:erickerickson@gmail.com]
>> *Sent:* Thursday, February 25, 2010 5:27 PM
>> *To:* java-dev@lucene.apache.org
>> *Subject:* Re: [jira] Commented: (LUCENE-2285) Code cleanup from all
>> sorts of (trivial) warnings
>>
>>
>>
>> Junit4:
>>
>>
>>
>> Well, simply disliking the @Test annotation seems like a poor reason to
>> stay with Junit3, although I admit it's a pain in the neck to change. Which
>> is why I didn't try to change all of them. The current system lends itself
>> to the practice of mangling the test name as a way of not running it, which
>> far too easily allows the test case to be forever ignored. One concrete
>> advantage of  annotations in Junit4 is the ability to add another "stupid"
>> annotation @Ignore, which then gets reported and thus doesn't get lost.
>>
>> As I remember, that last place we left localization what that Mike (?) saw
>> some intermittent problem that I couldn't reproduce. I could dust off that
>> code and see what the current state of affairs is since this has come up
>> again. The other problem was that the implementation I used lead to
>> *increased* test run times. The localization tests basically spun through
>> all the Locales available and ran all the tests in the class against them.
>> The current system only runs *some* of the tests in a test class through the
>> localization process. This can be addressed by, at worst, splitting the test
>> class up, but in my proof-of-concept that seemed like too much detail...
>>
>>
>>
>> My purpose in deprecating LuceneTestCase was to explicitly encourage
>> migration to Junit4, the deprecation warnings being the goad. I vote against
>> removing it....
>>
>>
>>
>> FWIW
>>
>> Erick
>>
>>
>>
>> On Thu, Feb 25, 2010 at 10:54 AM, Uwe Schindler (JIRA) <jira@apache.org>
>> wrote:
>>
>>
>>    [
>> https://issues.apache.org/jira/browse/LUCENE-2285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838384#action_12838384]
>>
>>
>> Uwe Schindler commented on LUCENE-2285:
>> ---------------------------------------
>>
>> Hi Shai,
>>
>> I applied the patch to my checkout, so it will not get out-of date. As
>> mentioned before, I have to review each change, as on my first diagonal
>> look-around I found a removed cast in TestCharArraySet/Map that is important
>> to call the right method, without the cast the test would pass, but the
>> affected method is never called. I am also not want to remove some casts in
>> NumericRange and other parts, where the casts were added for more clearness
>> in code. Especially at some places without the cast it is not clear what
>> javac will do, so the cast is for more "security" even if not needed.
>>
>> So please excuse by complaints, but two people looking over such a large
>> patch is really needed.
>>
>> Thanks for the work! Uwe
>>
>>
>> > Code cleanup from all sorts of (trivial) warnings
>> > -------------------------------------------------
>> >
>> >                 Key: LUCENE-2285
>> >                 URL: https://issues.apache.org/jira/browse/LUCENE-2285
>> >             Project: Lucene - Java
>> >          Issue Type: Improvement
>> >            Reporter: Shai Erera
>>
>> >            Assignee: Uwe Schindler
>>
>> >            Priority: Minor
>> >             Fix For: 3.1
>> >
>> >         Attachments: LUCENE-2285.patch
>> >
>> >
>> > I would like to do some code cleanup and remove all sorts of trivial
>> warnings, like unnecessary casts, problems w/ javadocs, unused variables,
>> redundant null checks, unnecessary semicolon etc. These are all very trivial
>> and should not pose any problem.
>> > I'll create another issue for getting rid of deprecated code usage, like
>> LuceneTestCase and all sorts of deprecated constructors. That's also trivial
>> because it only affects Lucene code, but it's a different type of change.
>> > Another issue I'd like to create is about introducing more generics in
>> the code, where it's missing today - not changing existing API. There are
>> many places in the code like that.
>> > So, with you permission, I'll start with the trivial ones first, and
>> then move on to the others.
>>
>> --
>> This message is automatically generated by JIRA.
>> -
>> You can reply to this email to add a comment to the issue online.
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: java-dev-help@lucene.apache.org
>>
>>
>>
>
>

Mime
View raw message