-
Notifications
You must be signed in to change notification settings - Fork 6
Fixing Failing SDK HTTP Retries Unit Tests #177
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
Conversation
|
I don't really think its good practice to cover up actual issues with our sdk like this. Why these two tests, but not also What are the actual errors we are seeing? These tests are mocked so I don't see where the randomness is coming from. |
| name=DETECTOR_NAME, | ||
| query="Is there a dog?", | ||
| confidence_threshold=DEFAULT_CONFIDENCE_THRESHOLD, | ||
| ) |
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.
^ This test is pretty weird, its attempting to create a detector w/ an existing name, which will fail every time on the backend. Then its overwriting what the backend returns w/ its own error codes in run_test.
|
It also doesnt look like these flaky settings fix the issue: https://github.com/groundlight/python-sdk/actions/runs/8026388146/job/21928766192 Could be because the underlying failures are correlated. |
test/unit/test_http_retries.py
Outdated
| ) | ||
|
|
||
|
|
||
| @flaky(max_runs=4, min_passes=1) |
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.
Not quite sure whats going on here, but this one is definitely hitting our actual backend somehow failing with a 400:
+ ERROR test/unit/test_http_retries.py::test_get_detector_attempts_retries - openapi_client.exceptions.ApiException: (400)
+ Reason: Bad Request
+ HTTP response headers: HTTPHeaderDict({'Date': 'Thu, 15 Feb 2024 22:39:21 GMT', 'Content-Type': 'application/json', 'Content-Length': '80', 'Connection': 'keep-alive', 'Server': 'nginx/1.21.1', 'Vary': 'Accept, Origin, Cookie', 'Allow': 'GET, POST, HEAD, OPTIONS', 'X-Frame-Options': 'DENY', 'X-Content-Type-Options': 'nosniff', 'Referrer-Policy': 'same-origin', 'Cross-Origin-Opener-Policy': 'same-origin', 'X-Request-Id-Resolved': 'req_uu47ba6a13a9df4f5b9676e61e87f32476', 'Set-Cookie': 'sessionid=damdm7m4g9wz2glez8vepj7m3o0n9d6l; expires=Thu, 29 Feb 2024 22:39:21 GMT; HttpOnly; Max-Age=1209600; Path=/; SameSite=Lax'})
+ HTTP response body: ["{'__all__': ['Constraint “unique_undeleted_name_per_set” is violated.']}"]
Which makes me think we're somehow missing the mark with this mock. Might be worth using pdb to check if we are actually mocking the call we think we are mocking.
… into sdk-retries-tests
One flaky test linked in the issue (
test_get_or_create_detector_attempts_retries) was just mocking the wrong function (urllib3.PoolManager.requestinstead ofrequests.request). This fixes that.The second test,
test_get_detector_attempts_retries, fails sometimes withunique_undeleted_name_per_setconstraint.In any test run, I haven't been able to reproduce the error, but should it re-occur I'll dig into it again.
This shows the integration tests passing for the first one after using
requests.request: https://github.com/groundlight/python-sdk/actions/runs/8072992972