-
Notifications
You must be signed in to change notification settings - Fork 25k
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.
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
Move query builder caching check to dedicated tests #43238
Conversation
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
Pinging @elastic/es-search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minor comments but the change looks good. We could maybe add a dedicated test for range queries since they can also disable caching if they contains now
and target a date
field ?
server/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/query/TermsSetQueryBuilderTests.java
Show resolved
Hide resolved
@jimczi thanks, added the two requested tests |
@elasticmachine run elasticsearch-ci/bwc |
@jimczi I also added the dedicated test for range queries that you requested, this should be ready for another review now. |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment, LGTM otherwise
server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/default-distro |
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
Thanks Jim, merged to master and 7.x |
Currently
AbstractQueryTestCase#testToQuery
checks the search context cachableflag. 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