Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Updating ESApi class methods to use ES client calls instead of temporary wrapper methods #3110
Updating ESApi class methods to use ES client calls instead of temporary wrapper methods #3110
Changes from 17 commits
c82e242
0d6e632
c9b27b3
204f0d5
f755efc
4c73640
e3bc2f0
1c84420
6b9b995
955ece3
7da0ff2
49a721f
8cd188f
1d85b85
b303f55
7a143b5
3e6a90f
c26a581
40065f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 see that we mock the calls to the API, call the API, and then check that the mock was called with the params we specified. It does not feel this tests anything other than the signature of the method. Is this typical in our tests?
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 saw a few tests that behave this way (here is an example of one), and I figured that because ESApi extends* the ES client, we could simply have coverage to make sure we are calling those classes correctly.
I can totally go back to the drawing board with these if we wanted to make them do more, though!
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.
We do test like this sometimes, alternative is testing with mocking HTTP requests: https://github.com/elastic/connectors/blob/main/tests/test_sink.py#L95
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 feel like the later we can mock, the better. Otherwise, we are testing things which are not likely to reveal a bug. The questions is why do we write the test? It should be to do sanity checking and to protect ourselves against an incompatible change, not just for the sake of tests. I defer to the team's judgement on the level of testing needed/
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.
Agree with your thoughts, Erik. These tests are rather thin, much like ESApi itself.
Given that ESApi acts like a partial wrapper around the ES client dependency, I would say any deeper testing would be akin to testing the client itself which, from connectors' perspective, is an import that presumably was tested already.
I don't think these unit tests are hurting anything overall, and they do follow precedent in how other tests are written in connectors. I am guaranteed to revisit this part of the code base when 9.0 releases, so I personally would advocate for these diffs as they sit.
The unit tests also technically don't exist yet, so I could be convinced we don't need them at all - we've come this far without them 🤣
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.
OK. If something changed in the underlying call and this test fails, it is worthy to keep. Your call
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 agree with Matt, these tests follow the pattern found in this repo and ensure that we pass arguments correctly to the client from our wrapper methods, which add the retry logic.
For the end-to-end scenario, we have a suite of functional tests that spin up an ES cluster and use these client calls to communicate with ES.
So, this code will ultimately be tested during the functional test step anyway.