Lucene - Core
  1. Lucene - Core
  2. LUCENE-2122

Use JUnit4 capabilites for more thorough Locale testing for classes deriving from LocalizedTestCase

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.1
    • Fix Version/s: 4.9, 5.0
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Use the @Parameterized capabilities of Junit4 to allow more extensive testing of Locales.

      1. LUCENE-2122.patch
        113 kB
        Robert Muir
      2. LUCENE-2122-r4.patch
        111 kB
        Erick Erickson
      3. LUCENE-2122-r3.patch
        111 kB
        Erick Erickson
      4. LUCENE-2122-r2.patch
        111 kB
        Erick Erickson
      5. LUCENE-2122.patch
        111 kB
        Erick Erickson

        Activity

        Hide
        Erick Erickson added a comment -

        All tests pass. This modifies all test classes (core and contrib) that derive from LocalizedTestCase. LocalizedTestCase now tests all test methods in all derived classes against all available Locales.

        If we want some of the tests to NOT run against all locales, we'd need to refactor them into their own test class....

        Show
        Erick Erickson added a comment - All tests pass. This modifies all test classes (core and contrib) that derive from LocalizedTestCase. LocalizedTestCase now tests all test methods in all derived classes against all available Locales. If we want some of the tests to NOT run against all locales, we'd need to refactor them into their own test class....
        Hide
        Robert Muir added a comment -

        Hi Erick, I am a little nervous about the change to LocalizedTestCase.tearDown() here.

        I think we must restore the users default Locale, since its a JRE-system wide global thing and we are changing it on the fly here.

        this was stashed away here before:

         /**
           * Before changing the default Locale, save the default Locale here so that it
           * can be restored.
           */
          private final Locale defaultLocale = Locale.getDefault();
        

        and restored in tearDown()... otherwise strange things could happen, such as your IDE could go bonkers after running the tests! (but maybe I am missing something)

        Show
        Robert Muir added a comment - Hi Erick, I am a little nervous about the change to LocalizedTestCase.tearDown() here. I think we must restore the users default Locale, since its a JRE-system wide global thing and we are changing it on the fly here. this was stashed away here before: /** * Before changing the default Locale, save the default Locale here so that it * can be restored. */ private final Locale defaultLocale = Locale.getDefault(); and restored in tearDown()... otherwise strange things could happen, such as your IDE could go bonkers after running the tests! (but maybe I am missing something)
        Hide
        Erick Erickson added a comment -

        Restoring original default Locale after test class has been run.

        Thanks Robert!

        Show
        Erick Erickson added a comment - Restoring original default Locale after test class has been run. Thanks Robert!
        Hide
        Robert Muir added a comment -

        Erick do you think LocalizedTestCase should be abstract?

        Show
        Robert Muir added a comment - Erick do you think LocalizedTestCase should be abstract?
        Hide
        Erick Erickson added a comment -

        Made LocalizedTestCase abstract...

        Show
        Erick Erickson added a comment - Made LocalizedTestCase abstract...
        Hide
        Robert Muir added a comment -

        Hi Erick, in the Date tools test I think you can delete the public static Collection<Locale[]> data(), I think you might have accidentally included it?

        Show
        Robert Muir added a comment - Hi Erick, in the Date tools test I think you can delete the public static Collection<Locale[]> data(), I think you might have accidentally included it?
        Hide
        Erick Erickson added a comment -

        OK, I plead advanced senility or some other excuse for the last patch.

        Robert:
        Thanks so much for looking this over, I have no clue what I was thinking with the TestDateTools. Or the other classes that derive from LocalizedTestCase.

        The @Parameterized and @RunWith only needed to be in LocalizedTestCase and all the inheriting classes just rely on the base class to collect the different locales.

        Anyway, this one should be much better....

        Erick

        Show
        Erick Erickson added a comment - OK, I plead advanced senility or some other excuse for the last patch. Robert: Thanks so much for looking this over, I have no clue what I was thinking with the TestDateTools. Or the other classes that derive from LocalizedTestCase. The @Parameterized and @RunWith only needed to be in LocalizedTestCase and all the inheriting classes just rely on the base class to collect the different locales. Anyway, this one should be much better.... Erick
        Hide
        Robert Muir added a comment -

        thanks Erick, i will play around with the patch some, generally just double-check the locale stuff is doing what we want, looks like it will.

        i havent tested yet, but looking at the code i have a few questions (i can try to add these to the patch just curious what you think):
        1. if a test fails under some locale, say th_TH, will junit 4 attempt to print this parameter out in some way so I know that it failed? If not do you know of a hack?
        2. i am thinking about reordering the locale array so that it tests the default one first. if you are trying to do some test-driven dev it might be strange if the test fails under a different locale first. I think this one is obvious, I will play with it to see how it behaves now.

        Show
        Robert Muir added a comment - thanks Erick, i will play around with the patch some, generally just double-check the locale stuff is doing what we want, looks like it will. i havent tested yet, but looking at the code i have a few questions (i can try to add these to the patch just curious what you think): 1. if a test fails under some locale, say th_TH, will junit 4 attempt to print this parameter out in some way so I know that it failed? If not do you know of a hack? 2. i am thinking about reordering the locale array so that it tests the default one first. if you are trying to do some test-driven dev it might be strange if the test fails under a different locale first. I think this one is obvious, I will play with it to see how it behaves now.
        Hide
        Robert Muir added a comment -

        Hi Erick, I played with this patch some and (not intentionally trying) I would get random test failures for TestQueryParser under eclipse... its not really something I am able to repeat though.

        maybe some race condition (I do not know how eclipse executes parameterized tests).... ?

        if it is a problem with my IDE that is one thing, just makes me a little nervous right now. trying to think what could cause this....

        Show
        Robert Muir added a comment - Hi Erick, I played with this patch some and (not intentionally trying) I would get random test failures for TestQueryParser under eclipse... its not really something I am able to repeat though. maybe some race condition (I do not know how eclipse executes parameterized tests).... ? if it is a problem with my IDE that is one thing, just makes me a little nervous right now. trying to think what could cause this....
        Hide
        Robert Muir added a comment -

        Hi Erick, I spent some time with this patch today and here is what happened:

        1. i tried to simplify localizedtestcase, so that it works just like the old one, with the exception of using junit4 parameterized facility.
        2. i wrote some bad tests and ensured things worked well such as error messages, default locale being run first, etc etc.
        3. i had everything good to go when i got random failures again, this time from 'ant clean test' about 3 times (pass,fail,pass)
        (sorry i should have done something to capture each test log but i did not)

        because the only real change here is use of the parameterized facility (the logic is the same), it makes me think that we should stick with .runBare() for the time being, because there is something strange going on here and I'm not even trying to break it.

        attached is the modified version of your patch.

        Show
        Robert Muir added a comment - Hi Erick, I spent some time with this patch today and here is what happened: 1. i tried to simplify localizedtestcase, so that it works just like the old one, with the exception of using junit4 parameterized facility. 2. i wrote some bad tests and ensured things worked well such as error messages, default locale being run first, etc etc. 3. i had everything good to go when i got random failures again, this time from 'ant clean test' about 3 times (pass,fail,pass) (sorry i should have done something to capture each test log but i did not) because the only real change here is use of the parameterized facility (the logic is the same), it makes me think that we should stick with .runBare() for the time being, because there is something strange going on here and I'm not even trying to break it. attached is the modified version of your patch.
        Hide
        Robert Muir added a comment -

        i am unassigning in case someone else can figure this one out, at my wits end here
        perhaps its just something wierd about my environment or something

        Show
        Robert Muir added a comment - i am unassigning in case someone else can figure this one out, at my wits end here perhaps its just something wierd about my environment or something
        Hide
        Robert Muir added a comment -

        btw, I left 'ant clean test' running in a loop and just checked it with this patch, no problems.
        so perhaps its my own incompetence. Erick can you take a look? Do you see some obvious problem?

        Show
        Robert Muir added a comment - btw, I left 'ant clean test' running in a loop and just checked it with this patch, no problems. so perhaps its my own incompetence. Erick can you take a look? Do you see some obvious problem?
        Hide
        Erick Erickson added a comment -

        Robert:

        Where are we on this? Last I knew your test problems had disappeared, but memory can be tricky....

        Show
        Erick Erickson added a comment - Robert: Where are we on this? Last I knew your test problems had disappeared, but memory can be tricky....
        Hide
        Robert Muir added a comment -

        Erick the test problems havent gone away (see latest hudson failure), but as you can see they appear to be unrelated to your patch, but existing problems.

        I am glad Uwe and Hudson are now seeing the failures I saw, but unfortunately I am unable to reproduce this problem. I think it is very wierd that it only recently has started happening and the Junit 4 jar file is looking very suspicious to me

        The next step is to force another test failure (with existing trunk code), except with an additional assertion in TestQueryParser.getParser(), that asserts that the created queryparser has the same Locale as LocalizedTestcase.locale

        If this assertion is not triggered, and a failure happens anyway, then there is no problem with LocalizedTestCase and we should commit your patch. This would mean the problem is instead some problem in the query parser, its tests, date calculations, jvm handling of thai dates, something else, but not the locale-switching.

        if the assertion is triggered, then we need to figure how why before changing the locale-swapping mechanism.

        I ran this test 2500 times last night but i couldnt make it fail. i have a very small portable right now and I will need to wait till the new year to really look at this more...

        Show
        Robert Muir added a comment - Erick the test problems havent gone away (see latest hudson failure), but as you can see they appear to be unrelated to your patch, but existing problems. I am glad Uwe and Hudson are now seeing the failures I saw, but unfortunately I am unable to reproduce this problem. I think it is very wierd that it only recently has started happening and the Junit 4 jar file is looking very suspicious to me The next step is to force another test failure (with existing trunk code), except with an additional assertion in TestQueryParser.getParser(), that asserts that the created queryparser has the same Locale as LocalizedTestcase.locale If this assertion is not triggered, and a failure happens anyway, then there is no problem with LocalizedTestCase and we should commit your patch. This would mean the problem is instead some problem in the query parser, its tests, date calculations, jvm handling of thai dates, something else, but not the locale-switching. if the assertion is triggered, then we need to figure how why before changing the locale-swapping mechanism. I ran this test 2500 times last night but i couldnt make it fail. i have a very small portable right now and I will need to wait till the new year to really look at this more...
        Hide
        Uwe Schindler added a comment -

        Some additional infos about the Hudson failure:
        In the past Hudson used an internal junit.jar to do the testing, which was in version 4 (the exact version can only be found out by one with hudson access, Mike?). So Hudson was always using Junit4 for testing even in Lucene 2.9. After the clover updates, we removed this JUnit lib path from the ANT command line and therefore also hudson uses 4.7 as shipped with the svn checkout.

        If it is really a JUnit problem, it seems to only occur with version 4.7. But I am not sure, I still stink it may be a thread starvation problem (slow test) or some local time problem that occurred two nights ago.

        Show
        Uwe Schindler added a comment - Some additional infos about the Hudson failure: In the past Hudson used an internal junit.jar to do the testing, which was in version 4 (the exact version can only be found out by one with hudson access, Mike?). So Hudson was always using Junit4 for testing even in Lucene 2.9. After the clover updates, we removed this JUnit lib path from the ANT command line and therefore also hudson uses 4.7 as shipped with the svn checkout. If it is really a JUnit problem, it seems to only occur with version 4.7. But I am not sure, I still stink it may be a thread starvation problem (slow test) or some local time problem that occurred two nights ago.
        Hide
        Robert Muir added a comment -

        I would be curious to know the previous version of the junit jar file that Hudson was using, if anyone can figure this out.

        I do not want to quickly blame junit or jvm, etc, but its very suspicious that this only recently started happening, and only sporadically for thai locale with the query parser.

        this will be a tricky one to figure out for sure.

        Show
        Robert Muir added a comment - I would be curious to know the previous version of the junit jar file that Hudson was using, if anyone can figure this out. I do not want to quickly blame junit or jvm, etc, but its very suspicious that this only recently started happening, and only sporadically for thai locale with the query parser. this will be a tricky one to figure out for sure.
        Hide
        Steve Rowe added a comment -

        Bulk move 4.4 issues to 4.5 and 5.0

        Show
        Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
        Hide
        Uwe Schindler added a comment -

        Move issue to Lucene 4.9.

        Show
        Uwe Schindler added a comment - Move issue to Lucene 4.9.

          People

          • Assignee:
            Unassigned
            Reporter:
            Erick Erickson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development