From a227941d20393ef58cb8cdb1d7821aeb8fafa931 Mon Sep 17 00:00:00 2001 From: Mike Pellegrini Date: Fri, 13 Sep 2024 11:48:01 -0400 Subject: [PATCH 1/3] Fix flaky test --- .../search/join/TestBlockJoinBulkScorer.java | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java index fe5e7e37517a..5f72b3ee188e 100644 --- a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java +++ b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java @@ -260,6 +260,15 @@ private static void assertScores( Float minScore, Map expectedScores) throws IOException { + assertScores(bulkScorer, scoreMode, minScore, List.of(expectedScores)); + } + + private static void assertScores( + BulkScorer bulkScorer, + org.apache.lucene.search.ScoreMode scoreMode, + Float minScore, + List> expectedScoresList) + throws IOException { Map actualScores = new HashMap<>(); bulkScorer.score( new LeafCollector() { @@ -283,7 +292,28 @@ public void collect(int doc) throws IOException { null, 0, NO_MORE_DOCS); - assertEquals(expectedScores, actualScores); + assertEqualsToOneOf(expectedScoresList, actualScores); + } + + private static void assertEqualsToOneOf(List expectedList, Object actual) { + boolean foundMatch = false; + for (Object expected : expectedList) { + if (expected == null) { + if (actual == null) { + foundMatch = true; + break; + } + } else { + foundMatch = expected.equals(actual); + if (foundMatch) { + break; + } + } + } + + if (!foundMatch) { + throw new AssertionError("expected one of: " + expectedList + " but was: " + actual); + } } public void testScoreRandomIndices() throws IOException { @@ -370,10 +400,18 @@ public void testSetMinCompetitiveScoreWithScoreModeMax() throws IOException { } { - // Doc 16 is returned because MaxScoreBulkScorer scores assuming A will match in doc 13, - // leading to a potential max score of 6. By the time it determines that A doesn't match, - // scoring is complete and thus there is no advantage to not collecting the doc. - Map expectedScores = + // This test case has two potential results. + // If all docs are scored in the same batch, then doc 16 is collected because + // MaxScoreBulkScorer scores assuming A will match in doc 13, leading to a potential max + // score of 6. + // If the scoring is split across two or more batches, then doc 16 is not collected + // because MaxScoreBulkScorer does not assume A will match in doc 13, leading to a + // potential max score of 5. + Map expectedScores1 = + Map.of( + 2, 6.0f, + 10, 10.0f); + Map expectedScores2 = Map.of( 2, 6.0f, 10, 10.0f, @@ -381,7 +419,7 @@ public void testSetMinCompetitiveScoreWithScoreModeMax() throws IOException { ScorerSupplier ss = weight.scorerSupplier(searcher.getIndexReader().leaves().get(0)); ss.setTopLevelScoringClause(); - assertScores(ss.bulkScorer(), scoreMode, 6.0f, expectedScores); + assertScores(ss.bulkScorer(), scoreMode, 6.0f, List.of(expectedScores1, expectedScores2)); } { From fe1671a18a5d4cbc2e896949f8f8ba6be3159317 Mon Sep 17 00:00:00 2001 From: Mike Pellegrini Date: Fri, 13 Sep 2024 12:11:37 -0400 Subject: [PATCH 2/3] Use assertEquals when list has one element --- .../apache/lucene/search/join/TestBlockJoinBulkScorer.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java index 5f72b3ee188e..6d05342b4c70 100644 --- a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java +++ b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java @@ -292,7 +292,12 @@ public void collect(int doc) throws IOException { null, 0, NO_MORE_DOCS); - assertEqualsToOneOf(expectedScoresList, actualScores); + + if (expectedScoresList.size() == 1) { + assertEquals(expectedScoresList.getFirst(), actualScores); + } else { + assertEqualsToOneOf(expectedScoresList, actualScores); + } } private static void assertEqualsToOneOf(List expectedList, Object actual) { From 6974209dd0686468fa948e65ca6dbd7ad0604d44 Mon Sep 17 00:00:00 2001 From: Mike Pellegrini Date: Mon, 16 Sep 2024 08:35:12 -0400 Subject: [PATCH 3/3] Use Objects.equals --- .../search/join/TestBlockJoinBulkScorer.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java index 6d05342b4c70..b9580331347f 100644 --- a/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java +++ b/lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinBulkScorer.java @@ -27,6 +27,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import org.apache.lucene.document.Document; import org.apache.lucene.document.Field; @@ -303,16 +304,9 @@ public void collect(int doc) throws IOException { private static void assertEqualsToOneOf(List expectedList, Object actual) { boolean foundMatch = false; for (Object expected : expectedList) { - if (expected == null) { - if (actual == null) { - foundMatch = true; - break; - } - } else { - foundMatch = expected.equals(actual); - if (foundMatch) { - break; - } + if (Objects.equals(expected, actual)) { + foundMatch = true; + break; } }