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

ESRestTestCase - assertWarnings does not work #36251

Closed
markharwood opened this issue Dec 5, 2018 · 5 comments
Closed

ESRestTestCase - assertWarnings does not work #36251

markharwood opened this issue Dec 5, 2018 · 5 comments
Labels
>bug >test Issues or PRs that are addressing/adding tests v7.0.0-beta1

Comments

@markharwood
Copy link
Contributor

When we deprecate a server endpoint, REST test clients start failing and there's no way to write tests that check the particular warnings issued.

Rest tests currently have a stark choice:

  • fail all uses of deprecated calls in a test class (setStrictDeprecationMode=true)
  • ignore all deprecation warnings in a test class (setStrictDeprecationMode=false)

What we'd ideally like is to be able to write tests that check the deprecated calls we make produce the warnings we'd expect and the ESTestCase base class already has a method to do just this: assertWarnings.

Sadly, assertWarnings currently only works in server-side tests. It relies on ThreadLocals populated by calls to DeprecationLogger deep down in the server call stack. These warnings do not get materialised in remote clients like ESRestTestCase. Ideally the Rest client would be able to take the HTTP warning headers it already knows about in Response.getWarnings() and make them available for checks in ESTestCase.assertWarnings. Maybe we could achieve that by also having a form of DeprecationLogger on the client? At the very least, assertWarnings should throw an Unsupported exception in ESRestTestCase if we don't have the means to propagate warnings.

Related: @nik9000 has #36126 in flight for LLRC which helps shift the failure strictness from a class level to a method level but I don't think that helps fulfil the goal of making EsRestTestCase.assertWarnings work as expected.

@markharwood markharwood added >bug >test Issues or PRs that are addressing/adding tests v7.0.0 :Core/Features/Java High Level REST Client labels Dec 5, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@nik9000
Copy link
Member

nik9000 commented Dec 5, 2018

I don't really like assertWarnings to be honest. I see it as a necessary evil when interacting with the ThreadLocal warnings. I just don't like interacting more with the thread local than I have to. I don't like how the warnings get "bunched up" and asserted at the end. Again, I don't think we have much choice for server side code. But for integration tests that work with external Elasticsearch clusters, I'd really prefer to assert the warnings on the response objects if possible, like we do with the yaml test framework. At least, theoretically like we do with the yaml test framework.

@markharwood
Copy link
Contributor Author

I'd really prefer to assert the warnings on the response objects if possible,

HLRC APIs currently return either existing "XxxxResponse" classes used in server or an equivalent HLRC-only "XxxxResponse" class. Long term the idea is to have no dependencies on server classes. So currently there is no common HLRC Response base class where "warnings" could be a common property. Until we make such a cutover we're stuck with ThreadLocals I expect.

@markharwood
Copy link
Contributor Author

After talking with Luca we came up with what looks like a better approach that works for LLRC and HLRC clients - see #36307

@markharwood
Copy link
Contributor Author

I closed #36307 which assumes single-version clusters.
Nik's new WarningsHandler #36345 now provides the means for more detailed inspection of warnings.
We can build on this with helper methods in ESRestTestCase to list the warnings we expect. What to expect will depend on whether the test is targeting a cluster all running the same-as-test-class version or whether it is a mixed-version cluster. The single-version cluster can list required warnings while the mixed-version can only list allowed warnings. EsRestTestCase has a nodeVersions set which we can use to figure out the single-vs-mixed version target cluster

markharwood added a commit to markharwood/elasticsearch that referenced this issue Dec 11, 2018
… and current-version-only clusters.

This is supported by a new VersionSpecificWarningsHandler class with associated unit test.
Closes elastic#36251
markharwood added a commit that referenced this issue Dec 11, 2018
…36443)

Added helper methods to ESRestTestCase for checking warnings in mixed and current-version-only clusters.
This is supported by a new VersionSpecificWarningsHandler class with associated unit test.
Closes #36251
markharwood added a commit that referenced this issue Dec 18, 2018
…36443)

Added helper methods to ESRestTestCase for checking warnings in mixed and current-version-only clusters.
This is supported by a new VersionSpecificWarningsHandler class with associated unit test.
Closes #36251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug >test Issues or PRs that are addressing/adding tests v7.0.0-beta1
Projects
None yet
Development

No branches or pull requests

4 participants