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 <u...@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: u...@thetaphi.de > > > > *From:* Erick Erickson [mailto:erickerick...@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) <j...@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-unsubscr...@lucene.apache.org > For additional commands, e-mail: java-dev-h...@lucene.apache.org > > >
