Lucene - Core
  1. Lucene - Core
  2. LUCENE-3784 Switching tests infrastructure to randomizedtesting.*
  3. LUCENE-3808

Switch LuceneTestCaseRunner to RandomizedRunner. Enforce Random sharing contracts. Enforce thread leaks.

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: general/test
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Dev. branch at: https://github.com/dweiss/lucene_solr/tree/rr

      Switch the runner to RandomizedRunner. Enforce the following:

      • Random sharing will result in a failure/ exception.
      • Add a validator for testXXX without @Test annotation. (custom test provider added).
      • Make sure tests are executed with assertions enabled (at least for solr/lucene packages).
      • Add a validator for static hook shadowing (no-no).
      • Modify custom execution groups in LTC to be real @Groups.

      Follow up from merging with trunk:

      • rename tests.threadspercpu into tests.jvms (Steve)
      • align to 80 cols terminals (Robert) - use simple class names and add truncation padding.
      • the output is emitted twice in case of suite-level errors.

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          Add a validator for testXXX without @Test annotation.

          Can we not require @Test? I think it should be fine to use testXXX without @Test. @Test is useless.

          Show
          Robert Muir added a comment - Add a validator for testXXX without @Test annotation. Can we not require @Test? I think it should be fine to use testXXX without @Test. @Test is useless.
          Hide
          Uwe Schindler added a comment -

          Dawid: Please let us name our tests like in JUnit3, our old test framework has a good test runner improvement for that, we don't like the verbosity on imports and this annotation.

          Show
          Uwe Schindler added a comment - Dawid: Please let us name our tests like in JUnit3, our old test framework has a good test runner improvement for that, we don't like the verbosity on imports and this annotation.
          Hide
          Erik Hatcher added a comment -

          Can we not require @Test? I think it should be fine to use testXXX without @Test. @Test is useless.

          +100000

          I was wondering if anyone would ever speak up about this. @Test is ridiculous. (so is @Override too, for that matter) - metadata is in the model implicitly.

          Show
          Erik Hatcher added a comment - Can we not require @Test? I think it should be fine to use testXXX without @Test. @Test is useless. +100000 I was wondering if anyone would ever speak up about this. @Test is ridiculous. (so is @Override too, for that matter) - metadata is in the model implicitly.
          Hide
          Dawid Weiss added a comment -

          Ok, ok, ok... I will not enforce @Test, don't kill me.

          Show
          Dawid Weiss added a comment - Ok, ok, ok... I will not enforce @Test, don't kill me.
          Hide
          Uwe Schindler added a comment -

          Show
          Uwe Schindler added a comment -
          Hide
          Uwe Schindler added a comment -

          I was wondering if anyone would ever speak up about this. @Test is ridiculous. (so is @Override too, for that matter) - metadata is in the model implicitly.

          @Override is indeed not really different but makes sense when you refactor classes. The big PRO is: no import required as its in java.lang. Thats the most horrible thing on @Test

          Show
          Uwe Schindler added a comment - I was wondering if anyone would ever speak up about this. @Test is ridiculous. (so is @Override too, for that matter) - metadata is in the model implicitly. @Override is indeed not really different but makes sense when you refactor classes. The big PRO is: no import required as its in java.lang. Thats the most horrible thing on @Test
          Hide
          Robert Muir added a comment -

          @Override is indeed not really different but makes sense when you refactor classes.

          Interfaces too

          Show
          Robert Muir added a comment - @Override is indeed not really different but makes sense when you refactor classes. Interfaces too
          Hide
          Dawid Weiss added a comment -

          I like @Override, I even wish it were an explicit keyword like in C# (although I tend to like Java's by-default virtual methods more than C#'s explicit virtuals).

          Show
          Dawid Weiss added a comment - I like @Override, I even wish it were an explicit keyword like in C# (although I tend to like Java's by-default virtual methods more than C#'s explicit virtuals).
          Hide
          Dawid Weiss added a comment -

          I've hacked a local branch where I substituted LuceneTestCaseRunner for RandomizedRunner. And boy, hell broke loose

          So... the biggest issue I'm facing is indeed with Random sharing across threads. One may argue that sharing a Random instance across threads (with data races) is in fact making it even more random, but this doesn't seem to be the direction consistent with the possibility of consistently re-running the tests given the same starting seed. I know it isn't possible to coordinate threads anyway but I would still like to have Random instances to return identical sequences from within a single thread's perspective.

          After this lengthy introduction, here comes an example of Random sharing:

             > Caused by: java.lang.RuntimeException: java.lang.IllegalStateException: This Random was created for/by another thread (Thread[RandomizedRunner-SuiteThread-org.apache.lucene.search.TestSearcherManager-seed#[3F1A26A6DA253628],5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]). Random instances must not be shared (acquire per-thread). Current thread: Thread[Thread-53,5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]
             > 	at __randomizedtesting.SeedInfo.seed([3F1A26A6DA253628]:0)
             > 	at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase$1.run(ThreadedIndexingAndSearchingTestCase.java:307)
             > Caused by: java.lang.IllegalStateException: This Random was created for/by another thread (Thread[RandomizedRunner-SuiteThread-org.apache.lucene.search.TestSearcherManager-seed#[3F1A26A6DA253628],5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]). Random instances must not be shared (acquire per-thread). Current thread: Thread[Thread-53,5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]
             > 	at com.carrotsearch.randomizedtesting.RandomNoSetSeed.checkValid(RandomNoSetSeed.java:124)
             > 	at com.carrotsearch.randomizedtesting.RandomNoSetSeed.nextInt(RandomNoSetSeed.java:72)
             > 	at org.apache.lucene.util.LuceneTestCase.rarely(LuceneTestCase.java:938)
             > 	at org.apache.lucene.analysis.MockAnalyzer.maybePayload(MockAnalyzer.java:102)
             > 	at org.apache.lucene.analysis.MockAnalyzer.createComponents(MockAnalyzer.java:95)
             > 	at org.apache.lucene.analysis.Analyzer.tokenStream(Analyzer.java:83)
             > 	at org.apache.lucene.document.Field.tokenStream(Field.java:467)
             > 	at org.apache.lucene.index.DocInverterPerField.processFields(DocInverterPerField.java:86)
             > 	at org.apache.lucene.index.DocFieldProcessor.processDocument(DocFieldProcessor.java:287)
             > 	at org.apache.lucene.index.DocumentsWriterPerThread.updateDocument(DocumentsWriterPerThread.java:241)
             > 	at org.apache.lucene.index.DocumentsWriter.updateDocument(DocumentsWriter.java:376)
             > 	at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:1533)
             > 	at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:1505)
             > 	at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase.updateDocument(ThreadedIndexingAndSearchingTestCase.java:111)
             > 	at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase$1.run(ThreadedIndexingAndSearchingTestCase.java:259)
             > Caused by: com.carrotsearch.randomizedtesting.NotAnException: Original allocation stack for this Random.
             > 	at java.lang.Thread.getStackTrace(Thread.java:1479)
             > 	at com.carrotsearch.randomizedtesting.RandomNoSetSeed.<init>(RandomNoSetSeed.java:26)
             > 	at com.carrotsearch.randomizedtesting.Randomness.<init>(Randomness.java:20)
             > 	at com.carrotsearch.randomizedtesting.Randomness.<init>(Randomness.java:24)
             > 	at com.carrotsearch.randomizedtesting.RandomizedRunner$3.run(RandomizedRunner.java:531)
             > 	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSuite(RandomizedRunner.java:551)
             > 	at com.carrotsearch.randomizedtesting.RandomizedRunner.access$3(RandomizedRunner.java:495)
             > 	at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:472)
          

          What's happening is that a Random is pushed to MockAnalyzer and then multiple threads access the same instance. I am investigating possible workarounds for this – Random instances are in fact thread local in RandomizedRunner so it would be possible to just grab your own Random in places like MockAnalyzer, without passing a ready to use instance.

          An ostrich solution is to dodge the issue and simply drop checking if a Random instance is shared or not, but it wouldn't be so much fun then, would it?

          Show
          Dawid Weiss added a comment - I've hacked a local branch where I substituted LuceneTestCaseRunner for RandomizedRunner. And boy, hell broke loose So... the biggest issue I'm facing is indeed with Random sharing across threads. One may argue that sharing a Random instance across threads (with data races) is in fact making it even more random, but this doesn't seem to be the direction consistent with the possibility of consistently re-running the tests given the same starting seed. I know it isn't possible to coordinate threads anyway but I would still like to have Random instances to return identical sequences from within a single thread's perspective. After this lengthy introduction, here comes an example of Random sharing: > Caused by: java.lang.RuntimeException: java.lang.IllegalStateException: This Random was created for/by another thread (Thread[RandomizedRunner-SuiteThread-org.apache.lucene.search.TestSearcherManager-seed#[3F1A26A6DA253628],5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]). Random instances must not be shared (acquire per-thread). Current thread: Thread[Thread-53,5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager] > at __randomizedtesting.SeedInfo.seed([3F1A26A6DA253628]:0) > at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase$1.run(ThreadedIndexingAndSearchingTestCase.java:307) > Caused by: java.lang.IllegalStateException: This Random was created for/by another thread (Thread[RandomizedRunner-SuiteThread-org.apache.lucene.search.TestSearcherManager-seed#[3F1A26A6DA253628],5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager]). Random instances must not be shared (acquire per-thread). Current thread: Thread[Thread-53,5,RandomizedRunner-SuiteThreadGroup-org.apache.lucene.search.TestSearcherManager] > at com.carrotsearch.randomizedtesting.RandomNoSetSeed.checkValid(RandomNoSetSeed.java:124) > at com.carrotsearch.randomizedtesting.RandomNoSetSeed.nextInt(RandomNoSetSeed.java:72) > at org.apache.lucene.util.LuceneTestCase.rarely(LuceneTestCase.java:938) > at org.apache.lucene.analysis.MockAnalyzer.maybePayload(MockAnalyzer.java:102) > at org.apache.lucene.analysis.MockAnalyzer.createComponents(MockAnalyzer.java:95) > at org.apache.lucene.analysis.Analyzer.tokenStream(Analyzer.java:83) > at org.apache.lucene.document.Field.tokenStream(Field.java:467) > at org.apache.lucene.index.DocInverterPerField.processFields(DocInverterPerField.java:86) > at org.apache.lucene.index.DocFieldProcessor.processDocument(DocFieldProcessor.java:287) > at org.apache.lucene.index.DocumentsWriterPerThread.updateDocument(DocumentsWriterPerThread.java:241) > at org.apache.lucene.index.DocumentsWriter.updateDocument(DocumentsWriter.java:376) > at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:1533) > at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:1505) > at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase.updateDocument(ThreadedIndexingAndSearchingTestCase.java:111) > at org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase$1.run(ThreadedIndexingAndSearchingTestCase.java:259) > Caused by: com.carrotsearch.randomizedtesting.NotAnException: Original allocation stack for this Random. > at java.lang.Thread.getStackTrace(Thread.java:1479) > at com.carrotsearch.randomizedtesting.RandomNoSetSeed.<init>(RandomNoSetSeed.java:26) > at com.carrotsearch.randomizedtesting.Randomness.<init>(Randomness.java:20) > at com.carrotsearch.randomizedtesting.Randomness.<init>(Randomness.java:24) > at com.carrotsearch.randomizedtesting.RandomizedRunner$3.run(RandomizedRunner.java:531) > at com.carrotsearch.randomizedtesting.RandomizedRunner.runSuite(RandomizedRunner.java:551) > at com.carrotsearch.randomizedtesting.RandomizedRunner.access$3(RandomizedRunner.java:495) > at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:472) What's happening is that a Random is pushed to MockAnalyzer and then multiple threads access the same instance. I am investigating possible workarounds for this – Random instances are in fact thread local in RandomizedRunner so it would be possible to just grab your own Random in places like MockAnalyzer, without passing a ready to use instance. An ostrich solution is to dodge the issue and simply drop checking if a Random instance is shared or not, but it wouldn't be so much fun then, would it?
          Hide
          Dawid Weiss added a comment -

          I've found this in the code:

              private final Random r;
              public MockIndexWriter(Random r, Directory dir, IndexWriterConfig conf) throws IOException {
                super(dir, conf);
                // must make a private random since our methods are
                // called from different threads; else test failures may
                // not be reproducible from the original seed
                this.r = new Random(r.nextInt());
          

          Ha!

          Show
          Dawid Weiss added a comment - I've found this in the code: private final Random r; public MockIndexWriter(Random r, Directory dir, IndexWriterConfig conf) throws IOException { super(dir, conf); // must make a private random since our methods are // called from different threads; else test failures may // not be reproducible from the original seed this.r = new Random(r.nextInt()); Ha!
          Hide
          Michael McCandless added a comment -

          So... the biggest issue I'm facing is indeed with Random sharing across threads.

          I think we should always (try to!) make a thread-private, consistently seed'd Random instance...

          True, the scheduling of the threads by the JVM will always be a source of non-determinism, but we shouldn't make that worse. And, for some bugs, having consistent thread-private Random will make the bug fully reproducible (ie if the bug isn't a thread scheduling issue).

          Show
          Michael McCandless added a comment - So... the biggest issue I'm facing is indeed with Random sharing across threads. I think we should always (try to!) make a thread-private, consistently seed'd Random instance... True, the scheduling of the threads by the JVM will always be a source of non-determinism, but we shouldn't make that worse. And, for some bugs, having consistent thread-private Random will make the bug fully reproducible (ie if the bug isn't a thread scheduling issue).
          Hide
          Dawid Weiss added a comment -

          I'm working on this on my github branch. This is a good test case for the runner itself.

          Show
          Dawid Weiss added a comment - I'm working on this on my github branch. This is a good test case for the runner itself.
          Hide
          Dawid Weiss added a comment -

          I've fixed all offending tests under lucene/
          https://github.com/dweiss/lucene_solr/commit/1d9a7effd06cfc0b393b7e00b8b7f755bd967700

          All tests pass with RandomizedRunner there and the output from tests that failed was an excellent source to quickly get at the offending place.

          This looks very good to me so far. Solr and modules next.

          Show
          Dawid Weiss added a comment - I've fixed all offending tests under lucene/ https://github.com/dweiss/lucene_solr/commit/1d9a7effd06cfc0b393b7e00b8b7f755bd967700 All tests pass with RandomizedRunner there and the output from tests that failed was an excellent source to quickly get at the offending place. This looks very good to me so far. Solr and modules next.
          Hide
          Dawid Weiss added a comment -

          I'm planning to merge github branched code into trunk this weekend. It's been running in parallel for some time now on my build server and it seems to have the same failure coverage and at the same time is a start to clean up LuceneTestCase and associated test code.

          I hope you'll also like the new infrastructure – will elaborate about this a bit once merged.

          Show
          Dawid Weiss added a comment - I'm planning to merge github branched code into trunk this weekend. It's been running in parallel for some time now on my build server and it seems to have the same failure coverage and at the same time is a start to clean up LuceneTestCase and associated test code. I hope you'll also like the new infrastructure – will elaborate about this a bit once merged.
          Hide
          Dawid Weiss added a comment -

          Moving remaining bullets to separate issues.

          Show
          Dawid Weiss added a comment - Moving remaining bullets to separate issues.
          Hide
          Robert Muir added a comment -

          minor spelling nit:

          [junit4] Ignoring seed attribute because it is overriden by user properties.

          overriden should be overridden...

          Show
          Robert Muir added a comment - minor spelling nit: [junit4] Ignoring seed attribute because it is overriden by user properties. overriden should be overridden...
          Hide
          Dawid Weiss added a comment -

          This wasn't a typo, I had it wrong all over the place. Corrected, thanks for pointing this out.

          Show
          Dawid Weiss added a comment - This wasn't a typo, I had it wrong all over the place. Corrected, thanks for pointing this out.

            People

            • Assignee:
              Dawid Weiss
              Reporter:
              Dawid Weiss
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development