zacharymorn commented on PR #972:
URL: https://github.com/apache/lucene/pull/972#issuecomment-1170684358

   
   > With this change, I suspect that some scorers created in `TestWANDScorer` 
would now use your new `BlockMaxMaxScoreScorer`, which is going to decrease the 
coverage of WANDScorer. Can we somehow make sure that `TestWANDScorer` always 
gets a `WANDScorer`? E.g. I spotted this query under 
`TestWANDScorer#testBasics` which likely uses your now scorer:
   > 
   > ```java
   >     //  test a filtered disjunction
   >     query =
   >         new BooleanQuery.Builder()
   >             .add(
   >                 new BooleanQuery.Builder()
   >                     .add(
   >                         new BoostQuery(
   >                             new ConstantScoreQuery(new TermQuery(new 
Term("foo", "A"))), 2),
   >                         Occur.SHOULD)
   >                     .add(new ConstantScoreQuery(new TermQuery(new 
Term("foo", "B"))), Occur.SHOULD)
   >                     .build(),
   >                 Occur.MUST)
   >             .add(new TermQuery(new Term("foo", "C")), Occur.FILTER)
   >             .build();
   > ```
   
   Yeah this is a good question. In my newly added tests I have used something 
like this to confirm it's testing the right scorer, but I'm not totally happy 
about this approach myself :
   ```
   if (scorer instanceof AssertingScorer) {
       assertTrue(((AssertingScorer) scorer).getIn() instanceof 
BlockMaxMaxscoreScorer);
   } else {
       assertTrue(scorer instanceof BlockMaxMaxscoreScorer);
   }
   ```
   
   One alternative approach could be instantiating `WANDScorer` directly inside 
the test for lower level tests, and moving the higher level tests into another 
test class that doesn't care about the specific scorer implementation for 
disjunction? This may require duplicating some code from `BooleanWeight`, 
`AssertingWeight` etc though but should be do-able.  
   
   On the other hand, if we don't plan on initiating `WANDScorer` directly in 
the test, varying the query clauses and asserting like above might be the best 
we could do I feel? This has the potential test coverage decrease issue as you 
suggested so may not be ideal either.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to