test(install): make github probe unit tests hermetic (mock requests.get)#2038
Merged
Conversation
Bump pyproject.toml and uv.lock to 0.24.0 and move the [Unreleased] CHANGELOG block to [0.24.0] - 2026-07-05 (one so-what entry per merged PR). MINOR bump: new `apm config list` alias (#1991) and Antigravity `trigger: glob` frontmatter support (#1984); no BREAKING changes. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.24.0 to trigger the release workflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…cessible The pre-flight accessibility probe treated a rate-limited 403/429 (primary 60/hr or secondary concurrency limit) as 'package not accessible' and aborted the install -- a false negative, since a throttled response is no evidence the repo is missing. On single-IP runners that concentrate many public-package installs (the consolidated macOS release job), this made the release/nightly integration suite flake red for days. Detect a genuine rate-limit response (403 + X-RateLimit-Remaining: 0, or 403/429 + Retry-After) and let the install proceed so the download step becomes the source of truth. Genuine 404s and permission 403s still fail closed. Adds unit coverage for the helpers and the allow-through / fail-closed end-to-end paths. Lint mirror green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
apm-spec-waiver: robustness bugfix -- throttled GitHub probe no longer false-negatives a public package; existing install existence-validation contract is unchanged (no new normative behaviour)
test_verbose_validation_failure_calls_build_error_context and test_github_host_skips_ssh_attempt mocked urllib.request.urlopen, but the validator uses requests.get -- so they made a real network call to api.github.com/repos/owner/repo and only passed because it returned 404. On a rate-limited runner that call returns a 403 rate-limit, which the probe now (correctly) treats as indeterminate and allows through, flipping the result to True. Mock requests.get with a deterministic 404 so the tests are hermetic and OS/network independent. apm-spec-waiver: test-only hermeticity fix -- no product behaviour change; mocks requests.get instead of the dead urllib path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two apm install unit tests that were unintentionally making live GitHub network calls by patching urllib.request.urlopen even though the install validation probe uses requests.get. By mocking apm_cli.install.validation.requests.get to return a deterministic 404 response, the tests become hermetic and stop flaking on rate-limited runners.
Changes:
- Replace dead
urllib.request.urlopenmocks withapm_cli.install.validation.requests.getmocks. - Make the GitHub accessibility probe behavior deterministic in the two affected unit tests (404 -> inaccessible ->
False).
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_install_command.py | Updates two unit tests to mock the real HTTP call site (requests.get) so they no longer depend on live GitHub responses. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+1191
to
1196
| with patch("apm_cli.install.validation.requests.get") as mock_get: | ||
| mock_get.return_value = MagicMock( | ||
| ok=False, status_code=404, reason="Not Found", headers={} | ||
| ) | ||
| result = _validate_package_exists("owner/repo", verbose=False) | ||
|
|
Comment on lines
+477
to
+480
| @patch("apm_cli.install.validation.requests.get") | ||
| def test_verbose_validation_failure_calls_build_error_context(self, mock_get, _mock_cred): | ||
| """When GitHub validation fails in verbose mode, build_error_context should be invoked.""" | ||
| import urllib.error | ||
|
|
||
| mock_urlopen.side_effect = urllib.error.HTTPError( | ||
| url="https://api.github.com/repos/owner/repo", | ||
| code=404, | ||
| msg="Not Found", | ||
| hdrs={}, | ||
| fp=None, | ||
| ) | ||
| mock_get.return_value = MagicMock(ok=False, status_code=404, reason="Not Found", headers={}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
TL;DR
Two unit tests in
tests/unit/test_install_command.pymockedurllib.request.urlopen, but the GitHub accessibility probe usesrequests.get. So they made a real network call toapi.github.com/repos/owner/repoand only passed because it returned 404. On the rate-limited macOS release runner that call returns a 403 rate-limit, which the probe now (correctly, per #2037) treats as indeterminate and allows through -- flipping the result toTrueand failing theassert result is False. This blocked the v0.24.0 release run's macOS unit-test step.Problem (WHY)
test_verbose_validation_failure_calls_build_error_contexttest_github_host_skips_ssh_attemptBoth patched the dead
urllibpath (a no-op forrequests-based code) and depended on a live 404 from GitHub. They were latently non-hermetic; the rate-limit hardening in #2037 exposed it. The macOS release job concentrates enough API calls on one IP to get throttled even during the unit phase, so the real probe returned 403 instead of 404.Approach (WHAT / HOW)
Mock
apm_cli.install.validation.requests.getto return a deterministic 404 response (ok=False, status_code=404, headers={}), matching each test's intent (404 -> not accessible ->False). The tests are now OS- and network-independent, and no longer flip under rate-limiting. No product code changes.Validation evidence
ruff check+ruff format --checkgreen on the changed file.How to test
Companion to #2037; both land in the v0.24.0 release tag.
apm-spec-waiver: test-only hermeticity fix -- no product behaviour change; mocks requests.get instead of the dead urllib path.