test(live): cover the quarantine add/remove round-trip - #1801
Merged
Conversation
Member
Author
|
This pull request is part of a Mergify stack:
|
Contributor
Merge Protections🟢 All 6 merge protections satisfied — ready to merge. Show 6 satisfied protections🟢 🤖 Continuous Integration
🟢 👀 Review Requirements
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 Reviews
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
The module header promised this file "deliberately mirrors the Python version 1:1 ... so the port can't drift the contract by accident", and named `func-tests/test_live_smoke.py` and `func-tests/conftest.py` as what it mirrors. Both were deleted when the port completed. Four helpers made the same promise individually (`Mirrors conftest.py::cli`, `Mirrors Python subprocess.run`, `Matches Python result.stdout + result.stderr`, and `live_token`'s "Mirrors Python `live_token` fixture"). AGENTS.md: "There is no Python: the port is complete ... If you find a doc, comment, or rule mentioning [it], it is stale — fix it." The harm is concrete rather than cosmetic: a maintainer auditing which secret each test needs goes looking for the fixture the header says pins that mapping, and there isn't one. The header now states the invariant that actually holds — tests are grouped by credential under a banner, and a test belongs under the banner matching its helper — which is the thing a reader needs and the thing the previous commit had to correct. It also spells `LIVE_TEST_MERGIFY_TOKEN_ADMIN` out rather than abbreviating it to `_ADMIN`, so grepping for either secret finds this file. Left alone: the in-body notes recording which wire contracts were preserved across the Python → Rust port. Those are provenance for why a contract is shaped the way it is, not claims about a file that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JBz3hxMUDCuWftT6qAHtnn Change-Id: Idd9e1e28bfc50b8b71d07c5938b4fa89af2745ff
jd
force-pushed
the
devs/jd/fix/live-tests-tests-show-admin-token/cover-quarantine-add-remove-round-trip--20ed0098
branch
from
September 7, 2026 12:10
fac01c2 to
d247f8e
Compare
Member
Author
Revision history
|
There was a problem hiding this comment.
🟡 Changes recommended
The new Drop-based cleanup logic currently tolerates the “not quarantined” case via substring matching without checking the expected exit code, which can mask unexpected cleanup failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds missing live smoke coverage for the tests quarantines add/remove mutations against the real Mergify API, preventing future auth/URL/payload regressions from slipping past wiremock-only tests.
Changes:
- Add
tests_quarantine_add_remove_roundtriplive test with RAII cleanup viaRemoveQuarantineOnDrop. - Extract shared randomness into
unique_suffix()and reuse it for the existing freeze round-trip test. - Minor comment wording tweak for the CI-Insights section header.
File summaries
| File | Description |
|---|---|
| crates/mergify-cli/tests/live_smoke.rs | Adds a quarantines add/remove live canary test with Drop-based cleanup; factors out unique suffix generation for concurrent-safe live runs. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
MRGFY-9001 dropped `ci_application_key` from `POST` and `DELETE` on
`/ci/{owner}/repositories/{repo}/quarantines` at the same time it
dropped it from `search/tests`. Only `search/tests` had a live
test, so only `search/tests` turned red; `mergify tests quarantines
add` and `remove` broke for every `ci` key with no signal at all.
The first two commits of this stack documented that. This closes
it.
The 20-odd wiremock tests in `tests_quarantine.rs` cannot cover
this: a mock server never authenticates, so no scope change is
visible to them. It has to be the live suite or nothing.
Shape follows `freeze_create_update_delete_roundtrip`, the existing
create-and-clean-up test in this file: a `Drop` guard removes the
entry so a failed assertion mid-test still leaves the canary
repository clean, and it warns rather than panics, because
panicking in `Drop` during an unwind aborts the process and buries
the assertion message that explains the failure. The guard is
registered before the response is parsed — an unparseable body
still means the row exists server-side.
Two details specific to quarantines:
- The quarantined name is `__mergify_cli_smoke_quarantine_<rand>__`
and matches nothing any real suite reports, so even a completely
skipped cleanup cannot suppress a genuine failure on the canary
repository. A leaked row is inert.
- Removal goes by name rather than by the id `add` returned, so one
run covers the list read that resolves the name *and* the delete.
The guard tolerates exactly two outcomes: exit 0, when the body
failed before its own remove, and `MergifyApiError` carrying
`not_found`'s `'<name>' is not quarantined`, when the body already
removed the row. Both halves are load-bearing — that exit code is
every Mergify API error including a 403 on a narrowed scope, and
the message on its own would swallow any failure whose text
happened to contain the phrase. It reads the code off
`mergify_core::ExitCode` rather than a literal, so a renumbering
cannot silently widen what cleanup calls success.
The 8-char entropy the freeze test generated inline is now
`unique_suffix()`, shared by both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Change-Id: I20ed0098fe86acd633ad6864eed035f6e5f47dd1
jd
force-pushed
the
devs/jd/fix/live-tests-tests-show-admin-token/cover-quarantine-add-remove-round-trip--20ed0098
branch
from
September 7, 2026 12:21
d247f8e to
4c11a93
Compare
jd
marked this pull request as ready for review
September 7, 2026 12:43
Base automatically changed from
devs/jd/fix/live-tests-tests-show-admin-token/drop-mirror-claims-deleted-python-files--dd9e1e28
to
main
September 7, 2026 13:28
kozlek
approved these changes
Sep 7, 2026
AlexandreGaubert
approved these changes
Sep 7, 2026
Contributor
Merge Queue Status
This pull request spent 9 minutes 14 seconds in the queue, including 8 minutes 22 seconds running CI. Required conditions to merge
|
40 tasks
mergify
Bot
deleted the
devs/jd/fix/live-tests-tests-show-admin-token/cover-quarantine-add-remove-round-trip--20ed0098
branch
September 7, 2026 13:57
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.
MRGFY-9001 dropped
ci_application_keyfromPOSTandDELETEon/ci/{owner}/repositories/{repo}/quarantinesat the same time itdropped it from
search/tests. Onlysearch/testshad a livetest, so only
search/teststurned red;mergify tests quarantines addandremovebroke for everycikey with no signal at all.The first two commits of this stack documented that. This closes
it.
The 20-odd wiremock tests in
tests_quarantine.rscannot coverthis: a mock server never authenticates, so no scope change is
visible to them. It has to be the live suite or nothing.
Shape follows
freeze_create_update_delete_roundtrip, the existingcreate-and-clean-up test in this file: a
Dropguard removes theentry so a failed assertion mid-test still leaves the canary
repository clean, and it warns rather than panics, because
panicking in
Dropduring an unwind aborts the process and buriesthe assertion message that explains the failure. The guard is
registered before the response is parsed — an unparseable body
still means the row exists server-side.
Two details specific to quarantines:
__mergify_cli_smoke_quarantine_<rand>__and matches nothing any real suite reports, so even a completely
skipped cleanup cannot suppress a genuine failure on the canary
repository. A leaked row is inert.
addreturned, so onerun covers the list read that resolves the name and the delete.
The guard tolerates exactly two outcomes: exit 0, when the body
failed before its own remove, and
MergifyApiErrorcarryingnot_found's'<name>' is not quarantined, when the body alreadyremoved the row. Both halves are load-bearing — that exit code is
every Mergify API error including a 403 on a narrowed scope, and
the message on its own would swallow any failure whose text
happened to contain the phrase. It reads the code off
mergify_core::ExitCoderather than a literal, so a renumberingcannot silently widen what cleanup calls success.
The 8-char entropy the freeze test generated inline is now
unique_suffix(), shared by both.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com