Add tests for building SuggestionSearchContext from suggestion builders

[cbuescher]

When we complete the current refactoring of the suggestion builders (#17096), we should add tests that check the output of the suggestion builders build() method, similar to what we did when refactoring the query builders. While we were refactoring the suggestion builders, we temporarily added tests that check that the results of parsing a builder in its xContent representation and calling the build() methods resulted in the same SuggestionSearchContext. After the deletion of SuggestParseElement we cannot use this kind of similarity check any more, so we need dedicated unit tests for that.

[jeff303]

I'm interested in taking a crack at this one. But I'm finding myself writing quite a bit of code to mock and stub out the necessary parts of the AnalysisService, SimilarityService, MapperRegistry, etc. that are needed in order to call SuggestBuilder.build. It makes me wonder whether I'm going about this in the right way. Does anyone know of any existing test case that provide instances that would be suitable here, or a good starting point?

[nik9000]

Does anyone know of any existing test case that provide instances that would be suitable here, or a good starting point?

AbstractQueryTestCase provides a lot of it, maybe all? Maybe good to figure out a way to reuse the code rather than copy it.

[cbuescher]

@jeff303 great that you want to look into this. What makes this kind of tests a bit tricky is that they should not require only minimal setup and mocking necessary for creating the SuggestionContext and then doing some meaningfull checks on it without having to really execute them. At least that was the idea, otherwise we are better off testing the output of the suggestion builder in integration tests like the existing SuggestSearchTest.

[jeff303]

Yeah, I was able to get it working pretty easily using @nik9000 suggestion, since AbstractQueryTestCase does provide a lot of the necessary setup. However, it is definitely heavier weight since it involves creating the cluster, index, etc. I am happy to investigate what the minimum set of objects that needs to be mocked is, especially now that I have something to start with. I mostly just wanted to make sure I wasn't going down the wrong path by missing something suitable that already exists in the other test classes.

[jeff303]

OK, I have what I believe to be the starting point now. I didn't end up needing any mocking. Also, I wasn't sure if it was important to assert the QueryShardContext objects themselves. If so, I could implement a deep equals (or else compare by reference in this particular case, but that seems to be of little value). I couldn't find a cleaner way to handle the MapperService other than as a class level field, since there's a bit of a chicken and egg issue with the MapperService needing a Supplier<QueryShardContext>, which needs a reference to the MapperService.

If anyone could take a quick look and let me know if I'm on the right track, I could fix/expand as needed before creating the pull request.

https://github.com/jeff303/elasticsearch/blob/fix/add-suggestion-search-context-tests/core/src/test/java/org/elasticsearch/search/suggest/SuggestionSearchContextTests.java

[cbuescher]

@jeff303 nice, I took a quick look and I think you are on the right track. However I think we should add tests for the output of the build() method to the already existing Phrase/Completion/TermSuggestionBuilderTests, possibly with a common setup in AbstractSuggestionBuilderTestCase. Those tests already provide ways of generating the suggestion builders with randomized parameters. The tests would call the public SuggestionContext build(QueryShardContext context) method on a random builder obtained by AbstractSuggestionBuilderTestCase#randomTestBuilder() (using a QueryShardContext that provides enough mocked services as needed) and then check that the builder pameters were correctly translated / transferred to the resulting SuggestionContext.

To better see what we had before, you can take a look at 4b803d7#diff-8877c8d8032280d9ca494007ca04b25aL239 and some of the assertions e.g. in 4b803d7#diff-7925b951f7645dcb8a552ff8440accd3L164

Those tests basically checked that when we moved the parsing logic from its previous location in the code to the build() methods we have now, we didn't break anything. However since we removed the original parsing logic, we want to check things like assertEquals(phraseSuggestionBuilder.gramSize(), phraseSuggestionContext.gramSize()), for as much parameters as this is feasible without too heavy mocking. Let me know if you have questions in case you want to continue working on this.

[jeff303]

@cbuescher , I'd appreciate it if you had a look and let me know if this is more on track (see PhraseSuggestionBuilderTests specifically). I'm not sure if I'm collecting the info in a great way from the random suggestion builders (and putting them in the MapperService), or if I could avoid adding the extra getters in DirectCandidateGeneratorBuilder and PhraseSuggestionBuilder.

master...jeff303:fix/add-suggestion-search-context-tests

[cbuescher]

Hi @jeff303, first of all thanks for working on this. I was about to leave a few comments on the code on your branch, but I can't seem to do this unless it's an official pull request, I'll add just a few thoughts here:

• adding the extra getters in DirectCandidateGeneratorBuilder and PhraseSuggestionBuilder should be okay, the paramters don't allow modification which is fine
• I briefly looked at the additional setup code and was wondering if all of this is really necessary for the tests that you are adding (not sure, might be). If in doubt, I would rather leave out testing all possible ways of e.g. setting analyzers in the suggester if that simplifies setup
• I'd try to avoid having to reset the mapper service (resetMapperService()) in AbstractSuggestionBuilderTestCase. You probably added this because the preFilter/postFilter analyzer names and fieldnames are randomly generated later with the random SuggestionBuilder. Maybe its possible to generate these analyzer and fieldnames upfront (e.g. a static array in PhraseSuggestionBuilderTests that the random setup draws from, or something similar)
• I'd investigate to pull up all checks for common SuggestionContext parameters (like size, analyzer, etc..) into the base test
• Whats the reason for having a dedicated SuggestionSearchContextTests? Currently it looks like lots of setup for just a few basic checks to me, those could probably be part of the general randomized tests, unless I'm missing something?

All in all I think this already looks like its going into a good direction, feel free to open a PR if you want to discuss details. Even if that PR turns out to need to be rewritten it provides room for discussion and detailed comments.

[jeff303]

OK, thanks for the feedback, @cbuescher . The SuggestionSearchContextTests class was just how I started out before switching over to the existing suggestion builder tests. I will remove that, and incorporate the other points and open an official PR.

[jeff303]

Unfortunately, I don't have the time to finish this right now, having just started a new full time job (among other things). I went ahead and pushed my latest changes (same branch as linked above), and I feel things are maybe 80% there.

[cbuescher]

Thanks @jeff303 for the update and all the effort so far. Since this is stalled a bit, would it be okay for somebody else to pick it up if she is interested? I'm just asking because this issue is still labeled "adoptme" which is meant to be a label that should encourage new contributers to take a look at open issues. If you have an idea if or when you'll be able to come back to this, we can also remove that label and assign it to you.