Repository navigation
test(bitbucket): add test coverage for defensive pagination and redirect guards - #1047
Conversation
potiuk
left a comment
There was a problem hiding this comment.
LGTM — three tests for three real guards, and they bite. Tests for defensive paths are easy to write in a way that passes whether or not the defence exists, so I checked by deleting each guard rather than by reading.
Scheme downgrade. _origin() returns (scheme, hostname, port), so an https → http redirect genuinely produces a different origin and trips SameHostRedirectHandler. The test pins the credential-forwarding refusal on the axis that matters most: a downgrade to plaintext is exactly when you least want the Authorization header to follow.
Non-advancing nextPageStart. Removing if next_start <= start: break from get_pull_request_commits makes your test fail — it blows up on StopIteration as the loop keeps asking for pages past the single mocked response, which is the infinite-loop behaviour the guard exists to prevent. assert len(opener.open.call_args_list) == 1 is what makes it precise: it pins one request, not merely a successful return.
Repeated next URL. Correctly targeted at the cloud paginator's own error.
Smaller observation — worth a follow-up, not a change here
While confirming the above I removed the same guard from get_repository_restrictions instead, and the entire 101-test suite still passed. So that sibling guard is completely unpinned.
datacenter.py repeats the next_start <= start pattern in six functions:
get_repository_restrictions list_open_pull_requests get_pull_request_commits
get_pull_request_status get_pull_request_reviews get_pull_request_discussion
This PR pins one of the six. cloud.py does not have that problem because its equivalent lives in a single _validated_next_url() helper, so one test covers every caller.
That contrast is the interesting part, and it points at a better follow-up than "write five more tests": hoisting the datacenter guard into a shared helper the way cloud already does would make one test cover all six, and would remove the possibility of the next paginator being added without it. Either way it is out of scope here — this PR strictly improves matters.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.
Summary
Adds unit test cases in
tools/bitbucket/tests/test_bitbucket.pyfor three defensive guards in the Bitbucket bridge to prevent regressions or accidental removal during future refactoring:test_same_host_redirect_handler_rejects_scheme_downgrade: AssertsSameHostRedirectHandlerrefuseshttps → httpredirects on the same host and raisesBitbucketError("refusing to forward credentials").test_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_start: Assertsdatacenter.get_pull_request_commitsterminates its pagination loop whennextPageStartis non-advancing (<= start), preventing infinite loops.test_cloud_get_pull_request_commits_rejects_repeated_next_url: Assertscloud.get_pull_request_commitsrejects repeated next URLs and raisesBitbucketError("pagination returned a repeated URL").Negative Control Verification
Each test was verified by temporarily disabling its guard in source code:
_origin()causedtest_same_host_redirect_handler_rejects_scheme_downgradeto fail (DID NOT RAISE BitbucketError).if next_start <= start: breakfromdatacenter.pycausedtest_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_startto fail (StopIteration).if next_url in seen_urls:fromcloud.pycausedtest_cloud_get_pull_request_commits_rejects_repeated_next_urlto fail (StopIteration).Test Suite Status
All 101 unit tests in
tools/bitbucketpass cleanly (uv run --directory tools/bitbucket pytest).fixes: #1007
Generated using Claude Code