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

Add support for assertWarnings() to ESRestTestCase #36260

Closed
wants to merge 2 commits into from

Conversation

markharwood
Copy link
Contributor

Added a "WarningsListener" to RestClient and ESRestTestCase provides such a listener so that it can use a DeprecationLogger to record warnings returned in HTTP responses.
Logging deprecation warnings causes the existing ThreadLocal storage of warning messages to take effect meaning EStTestCase.assertWarnings() calls have something to inspect.
Added an example use of assertWarnings to SearchIT

Closes #36251

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

Pinging @elastic/es-core-features

@javanna
Copy link
Member

javanna commented Dec 5, 2018

thanks @markharwood I see that the major changes are in the low-level REST client. Given that Response allows to retrieve warnings, I wonder if a WarningListener is needed in the client itself. For our purpose in ESRestTestCase could we instead wrap the RestClient and make sure that performRequest makes the additional call when warnings are returned? We may not even need the async variant given how the client is generally used in tests, not sure though. I would prefer not to add this feature directly to the client as it already exposes what's needed to implement it outside of it. Also, I find having to use a listener for blocking calls a bit weird. What do you think?

@markharwood
Copy link
Contributor Author

@nik9000 - I've been back and forward with @javanna on this PR and whether we should subclass RestClient to add in the "WarningListener" behaviour that picks up messages from Response.getWarnings().
If we subclass RestClient we'd need to change how it's constructed which may mean we want a custom RestClientBuilder too. The construction in EsRestTestCase.configureClient is shared by ESClientYamlSuiteTestCase too so maybe we want to consider how YAML tests approach warnings as part of this.

…r warnings surfacing up to ThreadLocal variables via DeprecationLogger
@nik9000
Copy link
Member

nik9000 commented Dec 5, 2018

I'd prefer that we use #36126 and check the warnings that come back from the request explicitly. I think assertWarnings is the best we can do for unit testing server code but we shouldn't use it if we don't absolutely have to because it bunches all of the warnings up until the end of the test case.

@markharwood
Copy link
Contributor Author

check the warnings that come back from the request explicitly

That works for LLRC calls but we have tests now that use HLRC and never see the LLRC Request/Response pairs generated under the covers.
Maybe we can have a base class check for HLRC-based tests where there's an assertWarnings that looks like this:

assertWarnings(Object hlrcResponse, String... warnings);

So behind the scenes the base class has an IdentityHashMap keyed on HLRC response objects (which can be anything, hence Object) and the values are warning messages held by any LLRC Response object that were generated under the covers. Only responses with response.hasWarning()==true have their warning messages added to the map.
The map is cleared after every test (like ESRestTest.after() does for ThreadLocal-based caches).

This base class (ESRestTestCase?) could override assertWarnings(String...) and throw an unsupported exception (we'd have to remove the final marker in EsTestCase) - I lost a chunk of time writing REST tests that expected that to work.

@markharwood
Copy link
Contributor Author

Closing - implemented using an alternative PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >test Issues or PRs that are addressing/adding tests v7.0.0-beta1 WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants