Updating ESApi class methods to use ES client calls instead of temporary wrapper methods #3110
Conversation
…f TemporaryConnectorApiWrapper methods
…sion of ES client releases
mattnowzari
changed the title
| @@ -255,3 +255,166 @@ async def test_get_all_docs(mock_responses): | |||
| assert doc_count == total | |||
|
|
|||
| await index.close() | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
|
buildkite test this |
1 similar comment
|
buildkite test this |
There was a problem hiding this comment.
LGTM, all of those client calls should be tested by the smoke test and functional tests in our CI pipeline.
The only exception is the code responsible for the filtering validation:
Can you follow docs and try setting up advanced sync rules for a connector that is managed by connector service from this branch, you can find some examples in this tutorial
If you manage add sync rule and see that framework is able to validate it or detect errors then feel free to merge
| @@ -255,3 +255,166 @@ async def test_get_all_docs(mock_responses): | |||
| assert doc_count == total | |||
|
|
|||
| await index.close() | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
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.
💔 Failed to create backport PR(s)
To backport manually run: |
…ary wrapper methods (#3110) Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit c08af2e) # Conflicts: # connectors/es/index.py
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Closes #2964
This PR updates the few methods of
class ESApito utilize ES client methods instead ofTemporaryConnectorAPIWrappermethods. This does not fully replace all of the methods ofTemporaryConnectorAPIWrapper, as we are waiting on ES client v8.17.1 to supportsync_job_claimandsync_job_update_statsAPI calls. This will be attended to post-8.17.1 release.Checklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)