sejal-pawar commented on pull request #159: URL: https://github.com/apache/lucene/pull/159#issuecomment-888702505
> Thanks @sejal-pawar! This is more what I was originally describing in the Jira issue. Thanks for updating your PR! > > I left some small comments on variable naming, javadoc, etc. Otherwise this seems pretty close to me. > > It would be nice to add a test case though around this new functionality. Maybe you could write a test that relies on the newly-exposed FacetsCollectors and computes a Facets result that is expected to agree with the Facets result exposed already? That would be a nice way to confirm the correct collectors are getting exposed (and don't regress somehow with a future change). Because there are a number of different cases here (many different implementations of the static `search` method), you could leverage Lucene's randomized testing to randomly invoke different code paths (e.g., randomly provide a CollectorManager vs. a Collector; randomly provide an ExecutorService to the ctor; etc.). > > I suppose instead of randomized testing, you could also add on some checks to the existing test cases that also grab the FacetsCollectors from the result and validate them against the Facets that are already tested. That might actually be the easiest way to go about the testing. Have a look in `TestDrillSideways` for what we do currently. Hey Greg, (apologies for the late reply!) I resolved the other comments but while writing the test, I noticed that a lot of test cases in DrillSidewaysResult involve the same logic for initialising DrillSideways. Ex. [1](https://code.amazon.com/packages/lucene/blobs/7a7003c51c8c0470f04e9df2ee9cb6002e124689/--/lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java#L1762) Would it perhaps make sense to extract the initialisation of DrillSideways into a helper test class called `DrillSidewaysInitialiser`? I was thinking of encapsulating all the required pieces like Directory, DirectoryTaxonomyWriter into a single class. Something similar has been done for document generation and initialisation in `org.apache.lucene.index.DocHelper`. -- 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
