-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Fix release tests after merging geogrid types #133929
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
Fix release tests after merging geogrid types #133929
Conversation
We run the tests for both release and snapshot, so we can see the different errors. This also increases the chance that when we move out of snapshot we wil see failures and need to fix the tests.
Pinging @elastic/es-analytical-engine (Team:Analytics) |
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.
Looks good, thanks! I have one comment that I think should be taken care of before merging, but please proceed at your own discretion.
report.humanReadable(true).prettyPrint(); | ||
report.startObject(); | ||
List<String> namesAndAliases = new ArrayList<>(DataType.namesAndAliases()); | ||
if (Build.current().isSnapshot() == false) { |
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'd replace this by a check for the specific capability. Otherwise, it's easy to forget to change this piece of code once that geotiles get enabled permanently.
Also applies to the other test fixes.
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.
Done
…elasticsearch into fix_release_tests_geogrid
The PR at #129581 did not carefully check that all tests also work as release tests. This lead to some CI failures, which we are fixing here.
Fixes #133886
Fixes #133887
Fixes #133888
Fixes #133889