apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.73k stars 1.05k forks source link

Use RandomMockMergePolicy more often [LUCENE-9469] #10508

Open asfimport opened 4 years ago

asfimport commented 4 years ago

During the work on #10005 we had some discussions about testability of the feature and how well we can guarantee that it's tested. One argument was that we can rely to a certain extend on the randomization and enabling these features by swapping in RandomMockMergePolicy. I ran a test which throws an assertion error every time the feature is explicitly used which required MockRandomMergePolicy to be used. Unfortunately I had two entire test runs without any failure except of the tests that explicitly enabled it ie. the ones that I wrote for the feature. I think we are not using this MP often enough in our tests and I want to propose to use it way more frequently. It's certainly not a replacement for dedicated unit tests, I wrote a dedicated one for every random failure I found which should be common practice but it would be great to increase coverage by swapping in MockRandomMergePolicy more often. Maybe something as simple as this would do it:

--- a/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java
+++ b/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java
`@@` -1059,7 +1059,7 `@@` public abstract class LuceneTestCase extends Assert {
   }

   public static MergePolicy newMergePolicy(Random r, boolean includeMockMP) {
-    if (includeMockMP && rarely(r)) {
+    if (includeMockMP && r.nextBoolean()) {
       return new MockRandomMergePolicy(r);
     } else if (r.nextBoolean()) {
       return newTieredMergePolicy(r);

Migrated from LUCENE-9469 by Simon Willnauer (@s1monw)

asfimport commented 4 years ago

Adrien Grand (@jpountz) (migrated from JIRA)

I think it has the potential to slow tests down significantly? If it doesn't +1 to do this change but otherwise we might need to make MockRandomMP find more reasonable merges as the index grows?