Return-Path: Delivered-To: apmail-lucene-java-dev-archive@www.apache.org Received: (qmail 92605 invoked from network); 25 Feb 2010 18:37:11 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3) by minotaur.apache.org with SMTP; 25 Feb 2010 18:37:11 -0000 Received: (qmail 57818 invoked by uid 500); 25 Feb 2010 18:37:10 -0000 Delivered-To: apmail-lucene-java-dev-archive@lucene.apache.org Received: (qmail 57784 invoked by uid 500); 25 Feb 2010 18:37:10 -0000 Mailing-List: contact java-dev-help@lucene.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: java-dev@lucene.apache.org Delivered-To: mailing list java-dev@lucene.apache.org Received: (qmail 57777 invoked by uid 99); 25 Feb 2010 18:37:10 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 25 Feb 2010 18:37:10 +0000 X-ASF-Spam-Status: No, hits=2.2 required=10.0 tests=HTML_MESSAGE,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of erickerickson@gmail.com designates 74.125.82.48 as permitted sender) Received: from [74.125.82.48] (HELO mail-ww0-f48.google.com) (74.125.82.48) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 25 Feb 2010 18:36:59 +0000 Received: by wwb34 with SMTP id 34so1982367wwb.35 for ; Thu, 25 Feb 2010 10:36:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type; bh=jj03YydLXEm1EXTSuUMvmRvW3CfUWy2wuYf9vKIg9iM=; b=GAtemrgwmZtcrlXlbrbY2flucIkU23vZyvswkiA2ggwdIjjzU9IPO1xXyDH1BcZQVO oegGBe4/RFCg0fPEYbiBYk3Txj7cumqzNJ3OMJ4/mzISp3rnWsyyafoliRKNuYwaX+O/ 7puSSzakT4nUMqMisrPbEJebUIPjjFuCSZ8WE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=NqqonfJGj0huPmttQiNJUz9+T0uBfN33VMXt5S1R2UhefblDXFJzyG0ZcWBK7gei7V eXTyLPtU3QIlhaavR45f0NtyansaAOko70ZGWrvNVlrIjF/H13TLkKsTTF6glr6TiXmQ m0xzrXRpcEJRimzjYsc1MubU7sllEtLFglm8M= MIME-Version: 1.0 Received: by 10.216.159.204 with SMTP id s54mr1048668wek.99.1267122999289; Thu, 25 Feb 2010 10:36:39 -0800 (PST) In-Reply-To: <786fde51002251027y37137756j3c31375aa066ddc0@mail.gmail.com> References: <441548908.516151267072227868.JavaMail.jira@brutus.apache.org> <737291194.526891267113267921.JavaMail.jira@brutus.apache.org> <359a92831002250826h55ac7444iec7109c2f0e738f7@mail.gmail.com> <000901cab63e$13b7f150$3b27d3f0$@de> <786fde51002251027y37137756j3c31375aa066ddc0@mail.gmail.com> Date: Thu, 25 Feb 2010 13:36:39 -0500 Message-ID: <359a92831002251036g4daee406sb27960a007d5727d@mail.gmail.com> Subject: Re: [jira] Commented: (LUCENE-2285) Code cleanup from all sorts of (trivial) warnings From: Erick Erickson To: java-dev@lucene.apache.org Content-Type: multipart/alternative; boundary=0016367fadd1426cbe04807110ff X-Virus-Checked: Checked by ClamAV on apache.org --0016367fadd1426cbe04807110ff Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable I don't have my heart set on keeping the deprecation, so taking it off work= s 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 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 i= t > should be such a big deal. > > @Ignore is a perfect other advantage of JUnit4. I've found some tests whi= ch > were prefixed with _, i.e. _testXYZ just to disable them. Nobody knows ab= out > it until he looks at the code (and pays attention). @Ignore would have be= en > 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 t= hem > 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/Aft= er, > 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 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 inten= t is >> to make the test runs faster, which we achieved by patching lots of test= s to >> not change static defaults and so be able to run all tests in the same J= VM >> 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 fie= lds >> 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 eve= n >> 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 LuceneTestCaseJ= 4 >> 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 =96 annotating a field that is n= ever >> used in code by an annotation is stupid and looks totally incorrect (I m= ean >> the field holding the TestWatchman-subclass). - This is another thing wh= y I >> am against the migration of our already proven tests. >> >> >> >> Because of that we don=92t want to deprecate LuceneTestCase and instead = only >> transform new tests and such needing @BeforeClass/@AfterClass for more s= peed >> 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. Wh= ich >> is why I didn't try to change all of them. The current system lends itse= lf >> to the practice of mangling the test name as a way of not running it, wh= ich >> far too easily allows the test case to be forever ignored. One concrete >> advantage of annotations in Junit4 is the ability to add another "stupi= d" >> annotation @Ignore, which then gets reported and thus doesn't get lost. >> >> As I remember, that last place we left localization what that Mike (?) s= aw >> some intermittent problem that I couldn't reproduce. I could dust off th= at >> 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 throug= h >> all the Locales available and ran all the tests in the class against the= m. >> 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 aga= inst >> removing it.... >> >> >> >> FWIW >> >> Erick >> >> >> >> On Thu, Feb 25, 2010 at 10:54 AM, Uwe Schindler (JIRA) >> wrote: >> >> >> [ >> https://issues.apache.org/jira/browse/LUCENE-2285?page=3Dcom.atlassian.j= ira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D128383= 84#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 impor= tant >> 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 clearn= ess >> 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 tri= vial >> and should not pose any problem. >> > I'll create another issue for getting rid of deprecated code usage, li= ke >> LuceneTestCase and all sorts of deprecated constructors. That's also tri= vial >> 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 ar= e >> 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 >> >> >> > > --0016367fadd1426cbe04807110ff Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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&= #39;s actually very easy to migrate all the current tests to JUnit 4 with t= he added import using some script. Even manually it should be such a big de= al.

@Ignore is a perfect other advantage of JUnit4. I've found some tes= ts which were prefixed with _, i.e. _testXYZ just to disable them. Nobody k= nows about it until he looks at the code (and pays attention). @Ignore woul= d have been better.

And there are lots of other advantages, like the @Before and @After (no= t only class). Another problem I've found in the tests is that not all = extended LuceneTestCase, and usually their setUp and tearDown implementatio= ns were wrong - not calling super first/last. When I moved them to extend L= uceneTestCase they broke (I fixed them, don't worry). However, that cou= ld never happen if the super's methods were tagged w/ @Before/After, be= cause 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 i= n 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 tha= t 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 sho= uld 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 &= lt;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 inte= nt is to make the test runs faster, which we achieved by patching lots of test= s 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 in= dex 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.

=A0

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 tru= nk 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 LuceneTestCas= eJ4 would be to emulate the old test* method names as if they have @Test. By th= at you could still disable them as mentioned, but it would reduce the burden o= f these dumb import statements and useless annotations.

=A0

By the way, why does LuceneTestCaseJ4 extend TestWatchman = and also a instance field extends that class? I do not understand the whole mag= ic behind, this is totally confusing to me =96 annotating a field that is neve= r 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.

=A0

Because of that we don=92t want to deprecate LuceneTestCas= e and instead only transform new tests and such needing @BeforeClass/@AfterClass = for more speed to the new API.

=A0

http://www.thetaphi.de

uwe@thetaphi.= de

From: Erick Erickson [mailto:ericke= rickson@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 s= orts of (trivial) warnings

=A0

Junit4:

=A0

Well, simply disliking = the @Test annotation seems like a poor reason to stay with Junit3, although I a= dmit it's a pain in the neck to change. Which is why I didn't try to cha= nge 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 =A0annotations in Junit4 is the ability to add another "stupid" annotation @Ignore, which then ge= ts 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 a= ll 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 tes= t class up, but in my proof-of-concept that seemed like too much detail...

=A0

My purpose in deprecating LuceneTestCase was to expl= icitly encourage migration to Junit4, the deprecation warnings being the goad. I v= ote against removing it....

=A0

FWIW

Erick

=A0

On Thu, Feb 25, 2010 at 10:54 AM, Uwe Schindler (JIR= A) <jira@apache.org= > wrote:


=A0 =A0[ https://issues.= apache.org/jira/browse/LUCENE-2285?page=3Dcom.atlassian.jira.plugin.system.= issuetabpanels:comment-tabpanel&focusedCommentId=3D12838384#action_1283= 8384 ]


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 importan= t to call the right method, without the cast the test would pass, but the affect= ed method is never called. I am also not want to remove some casts in NumericR= ange 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 pa= tch is really needed.

Thanks for the work! Uwe


> Code cleanup from all sorts of (trivial) warnings
> -------------------------------------------------
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 Key: LUCENE-2285
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 URL: https://issues.apache.org/ji= ra/browse/LUCENE-2285
> =A0 =A0 =A0 =A0 =A0 =A0 Project: Lucene - Java
> =A0 =A0 =A0 =A0 =A0Issue Type: Improvement
> =A0 =A0 =A0 =A0 =A0 =A0Reporter: Shai Erera

> =A0 =A0 =A0 =A0 =A0 =A0Assignee: Uwe Schindler

> =A0 =A0 =A0 =A0 =A0 =A0Priority: Minor
> =A0 =A0 =A0 =A0 =A0 =A0 Fix For: 3.1
>
> =A0 =A0 =A0 =A0 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 trivia= l 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 tr= ivial because it only affects Lucene code, but it's a different type of chang= e.
> Another issue I'd like to create is about introducing more generic= s in the code, where it's missing today - not changing existing API. There are m= any places in the code like that.
> So, with you permission, I'll start with the trivial ones first, a= nd 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

=A0



--0016367fadd1426cbe04807110ff--