-
Notifications
You must be signed in to change notification settings - Fork 602
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
Refactor ElasticsearchClient unit tests #9358
Conversation
10190be
to
deb73bf
Compare
I think the CodeQL stuff can be ignored for this review as it's just repeating the existing alerts (for some reason) 🤔 |
deb73bf
to
4cd7eb8
Compare
Now there's a flaky update test, but I don't see how this PR would have affected it. |
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.
@npepinpe nice 👍 LGTM 🚀
bors merge |
9358: Refactor ElasticsearchClient unit tests r=saig0 a=npepinpe ## Description This PR refactors the `ElasticsearchClient` unit tests to cover what the client is now responsible for: flushing and flushing logic, and communication with Elastic. This removes one unit tests which was previously in `ElasticsearchClientIT`, and removes the `ElasticsearchExporterFlushTest`, which are now simply nested in `ElasticsearchClientTest`. ## Related issues closes #9330 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
Build failed: |
Some flaky tests related to containers running out of memory :( bors retry |
9358: Refactor ElasticsearchClient unit tests r=saig0 a=npepinpe ## Description This PR refactors the `ElasticsearchClient` unit tests to cover what the client is now responsible for: flushing and flushing logic, and communication with Elastic. This removes one unit tests which was previously in `ElasticsearchClientIT`, and removes the `ElasticsearchExporterFlushTest`, which are now simply nested in `ElasticsearchClientTest`. ## Related issues closes #9330 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
Build failed: |
Same issue again :( Merging manually as bors is currently silent, and I'm pretty sure this PR did not affect the integration tests. |
Description
This PR refactors the
ElasticsearchClient
unit tests to cover what the client is now responsible for: flushing and flushing logic, and communication with Elastic.This removes one unit tests which was previously in
ElasticsearchClientIT
, and removes theElasticsearchExporterFlushTest
, which are now simply nested inElasticsearchClientTest
.Related issues
closes #9330
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.