Use newSearcher instead of new IndexSearcher in tests where possible #98110
Conversation
| simScorer = searcher.getSimilarity().scorer(boost, collectionStatistics, termStats.toArray(TermStatistics[]::new)); | ||
| approximationWeight = searcher.createWeight(approximate(in), ScoreMode.COMPLETE_NO_SCORES, 1f); | ||
| } else { | ||
| simScorer = null; |
There was a problem hiding this comment.
This change was necessary because the test now randomly wraps the similarity into an AssertingSimilarity which checks that the termStats array is not empty. This is not the case for the last part in SourceConfirmedTextQueryTests#testTerm, which is avoided by this additional condition.
| Fields luceneTermVectors = getTermVectorsFromLucene(directoryReader, test.doc); | ||
| validateResponse(response, luceneTermVectors, test); | ||
| } finally { | ||
| IOUtils.close(directoryReader); |
There was a problem hiding this comment.
Failing to close the reader can lead to thread leaks if the randomized searcher enables concurrency.
| Fields luceneTermVectors = getTermVectorsFromLucene(directoryReader, test.doc); | ||
| validateResponse(item.getResponse(), luceneTermVectors, test); | ||
| } finally { | ||
| IOUtils.close(directoryReader); |
There was a problem hiding this comment.
Failing to close the reader can lead to thread leaks if the randomized searcher enables concurrency.
cbuescher
added
>test
|
Pinging @elastic/es-search (Team:Search) |
|
@elasticmachine update branch |
|
@martijnvg could you have a look at the aggs related tests? There are some where we are now potentially adding concurrency to the mix, and I wonder if we may see precision errors etc. |
javanna
changed the title
| @@ -33,7 +33,7 @@ public void testNoData() throws Exception { | |||
| indexWriter.addDocument(Collections.singleton(new StringField("another_field", "value", Field.Store.NO))); | |||
| } | |||
| try (IndexReader reader = indexWriter.getReader()) { | |||
| IndexSearcher searcher = new IndexSearcher(reader); | |||
| IndexSearcher searcher = newSearcher(reader); | |||
There was a problem hiding this comment.
Looking deeper, I wonder if these aggs tests should rather use a ContextIndexSearcher
There was a problem hiding this comment.
I think we should do this as part of: #98672
I don't think this should block merging this PR. This is already an improvement.
As part elastic#98110, EngineTestCase now uses newSearcher() to create a new index searcher (instead of just creating a new index searcher). This is for increasing test coverage. However, this test can't deal with wrapping the index reader as it leads to assertion errors. So instead use `newSearcher(leaf, false)` to still keep to test benefits of wrapping the index searcher. Closes elastic#98716
|
Note that we changed a number of |
As part #98110, EngineTestCase now uses newSearcher() to create a new index searcher (instead of just creating a new index searcher). This is for increasing test coverage. However, this test can't deal with wrapping the index reader as it leads to assertion errors. So instead use `newSearcher(leaf, false)` to still keep to test benefits of wrapping the index searcher. Closes #98716
…her (#98932) Related to: #98110 Lucene test searcher will randomly wrap the passed reader. That means any usages that mix the two (the searcher and the previously reader), could cause tests to fail. Some of these locations maybe don't strictly need the searcher.getReader, but I just wanted to make sure we didn't have flaky tests. closes: #98925
This change swaps test code that directly creates IndexSearcher instances with
LuceneTestCase#newSearchercallsthat have the advantage of randomly using concurrency and also randomly use assertion wrappers internally.
While this doesn't guarantee testing the concurrent code path, it should generally increase the likelihood of doing so.