Migrate spark tests to mock_http fixture#23585
Migrate spark tests to mock_http fixture#23585mwdd146980 wants to merge 4 commits intomwdd146980/httpx-migration-basefrom
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: b877cb9 | Docs | Datadog PR Page | Give us feedback! |
`test_do_not_crash_on_version_collection_failure` simulates a network failure inside `_collect_version`. Production catches bare Exception, so the choice of exception type is cosmetic. Swap RequestException for HTTPRequestError to drop the last `requests` import from the test file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e690a234e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`unittest.mock` treats an exception class in a side_effect iterable as `raise cls`, which expands to `cls()`. `HTTPError.__init__` requires a `message` arg, so the bare class form raised TypeError instead of HTTPRequestError. Test still passed via the bare `except Exception` in `_collect_version`, but exercised the wrong path. Caught by codex review on PR 23585. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a327b159b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def test_auth_yarn(): | ||
| c = SparkCheck('spark', {}, [YARN_AUTH_CONFIG]) | ||
| assert c.http.options['auth'] == (TEST_USERNAME, TEST_PASSWORD) |
There was a problem hiding this comment.
This change turns test_auth_yarn into a constructor-only assertion (c.http.options['auth']) and no longer runs dd_run_check, so it no longer verifies the auth code path used during actual HTTP collection. A regression where Spark check requests stop applying auth at request time would now pass this test, whereas the previous version asserted auth on the request path and validated successful service checks under auth config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored the dd_run_check smoke run in b877cb9. Split into two tests because mock_http's PropertyMock on AgentCheck.http blocks reading wrapper options during the same run: test_auth_yarn_config asserts config propagates to c.http.options['auth'], test_auth_yarn asserts the check runs cleanly under auth config and produces OK service checks.
Wire-level "auth applied at request time" coverage is not restorable here. mock_http replaces the layer that applies auth on each request, so the wrapper-forwarding behavior cannot be observed under this fixture. That coverage belongs in datadog_checks_base where the wrapper itself is tested.
Adds back the dd_run_check + service-check assertions under YARN_AUTH_CONFIG that the migration dropped. Splits into two tests because mock_http's PropertyMock on AgentCheck.http blocks reading the real wrapper options during the same run. test_auth_yarn_config asserts config propagation; test_auth_yarn asserts the check runs cleanly under auth config. Wire-level auth verification is impossible with mock_http (the layer that applies auth on each request is exactly what mock_http replaces); that coverage belongs in the wrapper's own test suite in datadog_checks_base. Caught by codex review on PR 23585. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Validation ReportAll 20 validations passed. Show details
|
1 participant
Motivation
Step 5.4 of the httpx migration. Removes every
mock.patch('requests.Session.*')andmocker.patch('requests.Session.*')fromspark/tests/test_spark.pyso the test layer does not anchor torequests.Approach
Each
mock.patch('requests.Session.get', helper)becomesmock_http.get.side_effect = helper. One site usesreturn_valueinstead. The seven mock helpers drop their leadingsessionarg since the fixture no longer passes one through, which also lets the proxy helper drop its*args[1:]shim. Connection-failure tests raiseHTTPConnectionErrorfromdatadog_checks.base.utils.http_exceptionsinstead ofrequests.ConnectionError. Spark production already catches both atspark.py:687-706, so behavior is preserved.test_auth_yarnpreviously ran the full check under a custom helper that assertedauth in kwargsand verified service checks on the auth path. The auth-kwarg observation lived in the wrapper layer thatmock_httpbypasses. Coverage is split across two tests now.test_auth_yarn_configconstructs the check without the fixture and assertsc.http.options['auth']propagates from instance config.test_auth_yarnkeeps the smoke run withmock_httpplus service-check assertions underYARN_AUTH_CONFIG.The split is required because
mock_http'sPropertyMockonAgentCheck.httpblocks reading the real wrapper options during the same run. Wire-level "auth applied per request" coverage is not restorable undermock_httpand belongs in the wrapper's own tests indatadog_checks_base. Theyarn_requests_auth_mockhelper is removed.The last
from requests importdrovetest_do_not_crash_on_version_collection_failure'sside_effect=[RequestException, []]. Now usesHTTPRequestError("test failure"). Instantiation is required becauseunittest.mockraisescls()andHTTPError.__init__requiresmessage. Production catches bareExceptionthere, so the exception type is illustrative.Verification
ddev test -fs sparkclean.ddev --no-interactive test spark: 57 passed, 1 skipped. The threetest_integration_*errors requiredd_environmentand are unchanged from baseline.rg "requests" spark/tests/test_spark.pyreturns zero matches.Plan
PR 5.4 Spark Test Migration Plan
🤖 Generated with Claude Code