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.

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

[#30405] Adding tests for QueryExplanation - Backwards compatability of serialization #33111

Closed

Conversation

mirkojotic
Copy link
Contributor

Solves #30405

I might have gone in a completely wrong direction here and I'm willing to work on it. But in essence the way I understood the problem is there were no tests asserting that versions older than 6.4.0 would be using a different StreamInput / StreamOutput methods than on or after it.

- Backwards compatability of serialization
@colings86 colings86 added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories labels Aug 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@javanna
Copy link
Member

javanna commented Sep 18, 2018

@pcsanwald would you like to have a look at this one given that it solves an issue that you originally opened?

@pcsanwald
Copy link
Contributor

@javanna this addresses the original problem that @jpountz pointed out in #30390, which is that nothing broke when we changed the backward compat requirements for QueryExplanation. I tested this PR by pulling the branch, ensuring test passes, changing the compat string in QueryExplanation to Version.V_6_5_0, and ensuring that the test then fails:

> Task :server:test FAILED
Tests with failures:
  - org.elasticsearch.action.admin.indices.validate.query.QueryExplanationTests.testCompatabilityOfStreamInputApi_V_6_4_0
  - org.elasticsearch.action.admin.indices.validate.query.QueryExplanationTests.testCompatabilityOfStreamOutputApi_V_6_4_0

Copy link
Contributor

@pcsanwald pcsanwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested compatibility changes and it behaves as we'd expect. Thank you for contributing this!

@polyfractal
Copy link
Contributor

@elasticmachine test this

@cbuescher
Copy link
Member

@pcsanwald going through open PRs, this one seems outdated at least when targeting master.
The serialization for different versions tested here is no longer there on master. We could backport
the V_6_4_0 tests to 7.x but I'm not sure this is necessary. Would you be okay to close?

@mirkojotic sorry this slipped through the cracks somehow. Might backport this partially to 7.x if @pcsanwald
thinks that makes sense.

@pcsanwald
Copy link
Contributor

ok to close, thanks @cbuescher

@cbuescher cbuescher closed this Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants