-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Test: Enable strict deprecation on all tests #36558
Conversation
This drops the option for tests to disable strict deprecation mode in the low level rest client in favor of configuring expected warnings on any calls that should expect warnings. This behavior is paranoid-by-default which is generally the right way to handle deprecations and tests in general.
Pinging @elastic/es-core-infra |
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.
Thanks @nik9000, I left some minor comments.
Request request = new Request("HEAD", url); | ||
for (Map.Entry<String, String> param : params.entrySet()) { | ||
request.addParameter(param.getKey(), param.getValue()); | ||
} | ||
request.setOptions(expectVersionSpecificWarnings(w -> { |
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.
In #36511 I added expectWarnings
, which is syntactic sugar for this call. It might be nice to switch to that when it merges (should be very soon).
headTestCase("/test/test/1", emptyMap(), greaterThan(0)); | ||
headTestCase("/test/test/1", singletonMap("pretty", "true"), greaterThan(0)); | ||
headTestCase("/test/test/2", emptyMap(), NOT_FOUND.getStatus(), greaterThan(0)); | ||
headTestCase("/test/test/1", emptyMap(), OK.getStatus(), greaterThan(0), |
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.
Maybe we could switch to the _doc
type in this test, so that we don't need to check deprecation warnings for get requests. The main purpose of the tests is to check how HEAD
is handled, and not verify deprecation warnings (we do that in other places), so it could be nice to bring the test in line with the new API.
@jtibshirani, sorry for the long delay! Could you have another look at this? |
@elasticmachine, run elasticsearch-ci/docs-check |
@elasticmachine, run elasticsearch-ci/docbldesx |
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 to me!
Thanks for reviewing @jtibshirani! |
* master: Expose retention leases in shard stats (elastic#37991) Make primary terms fields private in index shard (elastic#38036) ML: Add reason field in JobTaskState (elastic#38029) Log flush_stats and commit_stats in testMaybeFlush HLRC: Fix strict setting exception handling (elastic#37247) Test: Enable strict deprecation on all tests (elastic#36558) Removes typed calls from YAML REST tests (elastic#37611) Switch default time format for ingest from Joda to Java for v7 (elastic#37934) Remove deprecated Plugin#onModule extension points (elastic#37866) Geo: Fix Empty Geometry Collection Handling (elastic#37978)
This drops the option for tests to disable strict deprecation mode in
the low level rest client in favor of configuring expected warnings on
any calls that should expect warnings. This behavior is
paranoid-by-default which is generally the right way to handle
deprecations and tests in general.