Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add dedicated Q.B. cache tests instead of generic testing in AbstractQueryTestCase#testToQuery #43200

Closed
cbuescher opened this issue Jun 13, 2019 · 2 comments · Fixed by #43238
Closed

Add dedicated Q.B. cache tests instead of generic testing in AbstractQueryTestCase#testToQuery #43200

cbuescher opened this issue Jun 13, 2019 · 2 comments · Fixed by #43238
Assignees
@cbuescher
Labels
:Search/Search Search-related issues that do not fall into other categories >test Issues or PRs that are addressing/adding tests v8.0.0-alpha1

Comments

@cbuescher
Copy link
Member

In relation to occasional failures like #43112 and while looking into a fix in #43199 I had some discussions
about if its really a good idea to test the caching behaviour of all query builders in general in
AbstractQueryTestCase#testToQuery like we currently do. Doing this for all kinds of randomized query builder
parametrizations requires sometimes non-trivial edge-case handling in either the query builder generation code
or the utility code that codifies assumtions about whether this particular configuration of a builder should be cached
or not (currently implemented on a test-by-test basis by overwriting isCachable()).

This has two drawbacks:

  • the "isCachable()" code can get quite complex even if the non-caching case is the exception
  • we might only rarely check the context cacheable flag because we only run into those cases accidentally

Instead we should probably remove the cache-flag checks from AbstractQueryTestCase#testToQuery and add dedicated,
more controlled tests for the query builders that need them.
@cbuescher cbuescher added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories v8.0.0 labels Jun 13, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@cbuescher
Copy link
Member Author

One potential disadvantage of removing the general checks from AbstractQueryTestCase#testToQuery would be that sub-test implementors might miss the need for dedicated cache tests, but that can maybe be enforced otherwise.

@cbuescher cbuescher self-assigned this Jun 14, 2019
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Jun 14, 2019
Move query builder caching check to dedicated tests
74a86e9 
Currently `AbstractQueryTestCase#testToQuery` checks the search context cachable
flag. This is a bit fragile due to the high randomization of query builders
performed by this general test. Also we might only rarely check the
"interesting" cases because they rarely get generated when fully randomizing the
query builder.

This change moved the general checks out ot #testToQuery and instead adds
dedicated cache tests for those query builders that exhibit something other than
the default behaviour.

Closes elastic#43200
@cbuescher cbuescher mentioned this issue Jun 14, 2019
Merged
cbuescher pushed a commit that referenced this issue Jun 27, 2019
Move query builder caching check to dedicated tests (#43238)
a4b97b6 
Currently `AbstractQueryTestCase#testToQuery` checks the search context cachable
flag. This is a bit fragile due to the high randomization of query builders
performed by this general test. Also we might only rarely check the
"interesting" cases because they rarely get generated when fully randomizing the
query builder.

This change moved the general checks out ot #testToQuery and instead adds
dedicated cache tests for those query builders that exhibit something other than
the default behaviour.

Closes #43200
cbuescher pushed a commit that referenced this issue Jun 27, 2019
Move query builder caching check to dedicated tests (#43238)
3636035 
Currently `AbstractQueryTestCase#testToQuery` checks the search context cachable
flag. This is a bit fragile due to the high randomization of query builders
performed by this general test. Also we might only rarely check the
"interesting" cases because they rarely get generated when fully randomizing the
query builder.

This change moved the general checks out ot #testToQuery and instead adds
dedicated cache tests for those query builders that exhibit something other than
the default behaviour.

Closes #43200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cbuescher cbuescher

Labels
:Search/Search Search-related issues that do not fall into other categories >test Issues or PRs that are addressing/adding tests v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move query builder caching check to dedicated tests cbuescher/elasticsearch
3 participants
@jakelandis @cbuescher @elasticmachine