Solr
  1. Solr
  2. SOLR-2673

Some Solr tests depend on order of test methods in source file, but Java 7 seems to no longer ensure that

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4, 4.0-ALPHA
    • Component/s: None
    • Labels:

      Description

      With Java7 (b147) Solr tests fail, seems to be because of caching (maybe ConcurrentLRUCache violates Java Memory Model, which can be seen in Java 7 only):

      Example TestFiltering (passes sometimes):

      junit-sequential:
          [junit] Testsuite: org.apache.solr.search.TestFiltering
          [junit] Tests run: 2, Failures: 0, Errors: 1, Time elapsed: 6,082 sec
          [junit]
          [junit] ------------- Standard Error -----------------
          [junit] VII 24, 2011 4:53:02 AM org.apache.solr.SolrTestCaseJ4 assertJQ
          [junit] SEVERE: query failed JSON validation. error=mismatch: '3'!='2' @ response/docs/[0]/val_i
          [junit]  expected =/response/docs==[{"val_i":3}]
          [junit]  response = {
          [junit]   "responseHeader":{
          [junit]     "status":0,
          [junit]     "QTime":11},
          [junit]   "response":{"numFound":5,"start":2,"docs":[
          [junit]       {
          [junit]         "val_i":2}]
          [junit]   }}
          [junit]  request = fl=val_i&sort=val_i+asc&start=2&q={!cache%3Dfalse}*:*&rows=1
          [junit] NOTE: reproduce with: ant test -Dtestcase=TestFiltering -Dtestmethod=testCaching -Dtests.seed=9127548164660185224:-55134
      75064094727964
          [junit] NOTE: test params are: codec=RandomCodecProvider: {id=MockFixedIntBlock(blockSize=571), val_i=MockFixedIntBlock(blockSiz
      e=571)}, locale=bg, timezone=Pacific/Galapagos
          [junit] NOTE: all tests run in this JVM:
          [junit] [TestFiltering]
          [junit] NOTE: Windows 7 6.1 amd64/Oracle Corporation 1.7.0 (64-bit)/cpus=2,threads=1,free=253309552,total=262799360
          [junit] ------------- ---------------- ---------------
          [junit] Testcase: testCaching(org.apache.solr.search.TestFiltering):        Caused an ERROR
          [junit] mismatch: '3'!='2' @ response/docs/[0]/val_i
          [junit] java.lang.RuntimeException: mismatch: '3'!='2' @ response/docs/[0]/val_i
          [junit]     at org.apache.solr.SolrTestCaseJ4.assertJQ(SolrTestCaseJ4.java:467)
          [junit]     at org.apache.solr.SolrTestCaseJ4.assertJQ(SolrTestCaseJ4.java:415)
          [junit]     at org.apache.solr.search.TestFiltering.testCaching(TestFiltering.java:105)
          [junit]     at org.apache.lucene.util.LuceneTestCase$LuceneTestCaseRunner.runChild(LuceneTestCase.java:1486)
          [junit]     at org.apache.lucene.util.LuceneTestCase$LuceneTestCaseRunner.runChild(LuceneTestCase.java:1404)
          [junit]
          [junit]
          [junit] Test org.apache.solr.search.TestFiltering FAILED
      

      It is definitely not a Hotspot issue, as it also happens with -Xint and -Xbatch.

      1. SOLR-2673.patch
        0.5 kB
        Uwe Schindler
      2. SOLR-2673_randomize_methods.patch
        2 kB
        Robert Muir
      3. SOLR-2673_randomize_methods.patch
        2 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          bulk close for 3.4

          Show
          Robert Muir added a comment - bulk close for 3.4
          Hide
          Robert Muir added a comment -

          i don't think thats very important, its just our internal tests, and everyone here is a committer.

          Show
          Robert Muir added a comment - i don't think thats very important, its just our internal tests, and everyone here is a committer.
          Hide
          Uwe Schindler added a comment -

          We should add a changes entry.

          Show
          Uwe Schindler added a comment - We should add a changes entry.
          Hide
          Robert Muir added a comment -

          I think we can resolve this issue now.

          Show
          Robert Muir added a comment - I think we can resolve this issue now.
          Hide
          Chris Male added a comment -

          +1

          Show
          Chris Male added a comment - +1
          Hide
          Robert Muir added a comment -

          updated patch, with the sort-by-name before shuffle.

          Show
          Robert Muir added a comment - updated patch, with the sort-by-name before shuffle.
          Hide
          Robert Muir added a comment -

          We must do this in all cases, because else the same random would not help, if Java 7 already shuffled them.

          You and chris are definitely right, in order to be consistent across different JREs, we must sort the methods first before shuffling.

          I'll update the patch.

          Show
          Robert Muir added a comment - We must do this in all cases, because else the same random would not help, if Java 7 already shuffled them. You and chris are definitely right, in order to be consistent across different JREs, we must sort the methods first before shuffling. I'll update the patch.
          Hide
          Robert Muir added a comment -

          attached is a patch that inits LTC's random inside computeTestMethods instead, and uses this to shuffle the test methods (so the shuffle is reproducible according to random seed)

          Show
          Robert Muir added a comment - attached is a patch that inits LTC's random inside computeTestMethods instead, and uses this to shuffle the test methods (so the shuffle is reproducible according to random seed)
          Hide
          Uwe Schindler added a comment -

          I doubt its truly random... it may be undefined but for a given jre its probably typically the same each time.

          Its somehow random in Java 7. If I run the same test multiple times, sometimes it fails sometimes it succeeds. Very strange, but it seems to be related to class loading and hotspot (maybe it uses some internal hotspot caches to speedup reflective access).

          Show
          Uwe Schindler added a comment - I doubt its truly random... it may be undefined but for a given jre its probably typically the same each time. Its somehow random in Java 7. If I run the same test multiple times, sometimes it fails sometimes it succeeds. Very strange, but it seems to be related to class loading and hotspot (maybe it uses some internal hotspot caches to speedup reflective access).
          Hide
          Chris Male added a comment -

          Yup.

          Show
          Chris Male added a comment - Yup.
          Hide
          Uwe Schindler added a comment -

          We could also sort it to a known order (alphabetically) and then shuffle.

          We must do this in all cases, because else the same random would not help, if Java 7 already shuffled them.

          Show
          Uwe Schindler added a comment - We could also sort it to a known order (alphabetically) and then shuffle. We must do this in all cases, because else the same random would not help, if Java 7 already shuffled them.
          Hide
          Uwe Schindler added a comment -

          First bunch fixed (TestFiltering and TestEchoParams):

          Committed trunk revision: 1150362
          Committed 3.x revision: 1150367

          Show
          Uwe Schindler added a comment - First bunch fixed (TestFiltering and TestEchoParams): Committed trunk revision: 1150362 Committed 3.x revision: 1150367
          Hide
          Chris Male added a comment -

          We could also sort it to a known order (alphabetically) and then shuffle.

          Show
          Chris Male added a comment - We could also sort it to a known order (alphabetically) and then shuffle.
          Hide
          Robert Muir added a comment -

          I doubt its truly random... it may be undefined but for a given jre its probably typically the same each time.

          I will investigate. We might be able to init LuceneTestCase's random in computeTestMethods instead of in beforeClass(), this would
          give us a reproducible shuffle, which is the ideal situation.

          Show
          Robert Muir added a comment - I doubt its truly random... it may be undefined but for a given jre its probably typically the same each time. I will investigate. We might be able to init LuceneTestCase's random in computeTestMethods instead of in beforeClass(), this would give us a reproducible shuffle, which is the ideal situation.
          Hide
          Chris Male added a comment -

          Absolutely. My point is, can we even avoid that since the initial Collection is itself randomly sorted now since the methods come back in an undefined order?

          Show
          Chris Male added a comment - Absolutely. My point is, can we even avoid that since the initial Collection is itself randomly sorted now since the methods come back in an undefined order?
          Hide
          Robert Muir added a comment -

          By the way, if you add this shuffle, all these problems become visible on java6.

          Also TestReplicationHandler is broken in the same way.

          Show
          Robert Muir added a comment - By the way, if you add this shuffle, all these problems become visible on java6. Also TestReplicationHandler is broken in the same way.
          Hide
          Robert Muir added a comment -

          The problem i am concerned about would be e.g. a test that makes a reader in @beforeclass (many tests build the index in beforeclass and share resources across tests).

          In this case, perhaps there is some reuse bug (e.g. imagine a bug in terms dictionary cache or something) and it would not be reproducible if we shuffled differently...

          Show
          Robert Muir added a comment - The problem i am concerned about would be e.g. a test that makes a reader in @beforeclass (many tests build the index in beforeclass and share resources across tests). In this case, perhaps there is some reuse bug (e.g. imagine a bug in terms dictionary cache or something) and it would not be reproducible if we shuffled differently...
          Hide
          Chris Male added a comment -

          If the Collection is ordered differently each time before the shuffle (which it might be since the method order is no longer defined), would it ever be reproducible even with a known random?

          Show
          Chris Male added a comment - If the Collection is ordered differently each time before the shuffle (which it might be since the method order is no longer defined), would it ever be reproducible even with a known random?
          Hide
          Robert Muir added a comment -

          hehe i want all tests to be reproducible though, maybe we pass shuffle a random init'ed by the current day of the month or something

          we don't have to commit it either, or we can make it a -D or something that is not set by default, just so we can fix tests.

          Show
          Robert Muir added a comment - hehe i want all tests to be reproducible though, maybe we pass shuffle a random init'ed by the current day of the month or something we don't have to commit it either, or we can make it a -D or something that is not set by default, just so we can fix tests.
          Hide
          Chris Male added a comment -

          Was just going to suggest the same thing Robert!

          Show
          Chris Male added a comment - Was just going to suggest the same thing Robert!
          Hide
          Uwe Schindler added a comment -

          Simple fix for TestFiltering.

          Show
          Uwe Schindler added a comment - Simple fix for TestFiltering.
          Hide
          Robert Muir added a comment -
          Index: lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java
          ===================================================================
          --- lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java	(revision 1150089)
          +++ lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java	(working copy)
          @@ -1457,6 +1457,7 @@
                     } catch (Exception e) { throw new RuntimeException(e); }
                   }
                 }
          +      Collections.shuffle(testMethods); // doesn't use seed (its not initialized yet!) but i think this is ok...
                 return testMethods;
               }
          
          Show
          Robert Muir added a comment - Index: lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java =================================================================== --- lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java (revision 1150089) +++ lucene/src/test-framework/org/apache/lucene/util/LuceneTestCase.java (working copy) @@ -1457,6 +1457,7 @@ } catch (Exception e) { throw new RuntimeException(e); } } } + Collections.shuffle(testMethods); // doesn't use seed (its not initialized yet!) but i think this is ok... return testMethods; }
          Hide
          Uwe Schindler added a comment -

          More info, so its really not defined in which order reflection returns fields:

          http://stackoverflow.com/questions/1097807/java-reflection-is-the-order-of-class-fields-and-methods-standardized

          This issue should fix all solr tests to no longer rely on order as declared in tests. So tests should have no side-effects. If they have side-effects, we should make a hyper-method that calls the previous test* methods in order. I did this for EchoParams and now it works.

          Show
          Uwe Schindler added a comment - More info, so its really not defined in which order reflection returns fields: http://stackoverflow.com/questions/1097807/java-reflection-is-the-order-of-class-fields-and-methods-standardized This issue should fix all solr tests to no longer rely on order as declared in tests. So tests should have no side-effects. If they have side-effects, we should make a hyper-method that calls the previous test* methods in order. I did this for EchoParams and now it works.
          Hide
          Dawid Weiss added a comment -

          Can you confirm this? If this is the case, it'll be interesting. There is nothing in Java spec saying reflection should return field or methods in their declaration order, but traditionally this was the case. Jeroen Frijters (IKVM) hit a similar problem with other Java programs – they rely on the order of fields returned by reflection; turned out he actually had to patch IKVM to mimic Java (HotSpot) behavior for these programs.

          http://weblog.ikvm.net/CommentView.aspx?guid=c1c354e6-2c7d-404a-b2fe-244db9f03250

          Show
          Dawid Weiss added a comment - Can you confirm this? If this is the case, it'll be interesting. There is nothing in Java spec saying reflection should return field or methods in their declaration order, but traditionally this was the case. Jeroen Frijters (IKVM) hit a similar problem with other Java programs – they rely on the order of fields returned by reflection; turned out he actually had to patch IKVM to mimic Java (HotSpot) behavior for these programs. http://weblog.ikvm.net/CommentView.aspx?guid=c1c354e6-2c7d-404a-b2fe-244db9f03250
          Hide
          Uwe Schindler added a comment -

          Aahhhhaaaa.

          We should always clear the index. It seems maybe that reflection and annotations in Java 7 are simply return (for performance reasons) in arbitrary order.

          In general it is bad to have them relying on each other.

          Show
          Uwe Schindler added a comment - Aahhhhaaaa. We should always clear the index. It seems maybe that reflection and annotations in Java 7 are simply return (for performance reasons) in arbitrary order. In general it is bad to have them relying on each other.
          Hide
          Yonik Seeley added a comment -

          Hmmm... notice the "numFound":5
          There are only 4 documents added in this test.
          If I add a clearIndex() at the top of the test method, everything is fine.

          But why would that be necessary?
          It looks like with Java7, that the test methods are not being run in the order they are declared in the file.
          That's probably the root cause of the other Solr test failures with Java7 too.

          Show
          Yonik Seeley added a comment - Hmmm... notice the "numFound":5 There are only 4 documents added in this test. If I add a clearIndex() at the top of the test method, everything is fine. But why would that be necessary? It looks like with Java7, that the test methods are not being run in the order they are declared in the file. That's probably the root cause of the other Solr test failures with Java7 too.
          Hide
          Uwe Schindler added a comment -

          I tried making all methods of ConcurrentLRUCache synchronized, it did not help. So problem seems to be somewhere else.

          Show
          Uwe Schindler added a comment - I tried making all methods of ConcurrentLRUCache synchronized, it did not help. So problem seems to be somewhere else.

            People

            • Assignee:
              Unassigned
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development