Two findings in the --wait path of gr pr merge, both small, neither a behavior defect. Filing them because the path is exercised only in the case where it does nothing.
1. The polling loop is never entered by any test
--wait polls until every PR's checks resolve, or until --timeout elapses and the command fails.
The only test naming --wait is test_pr_merge_wait_does_not_block_when_no_checks_are_configured, which covers the case where no checks exist, so any_pending is false and the loop is skipped entirely. Nothing enters the loop, so none of this is covered:
- the timeout exit
- the sleep-and-repoll cycle
- the transition from pending to passing, and from pending to failing
- the
Err(_) arm that keeps a PR pending when a status query fails
Confirmed by mutation: neutering the timeout condition turns no test red.
The exit code of a timed-out wait is therefore unverified in both directions, which matters for a flag whose whole purpose is to be used unattended in scripts.
A stale-data defect was specifically ruled out. The loop's exit conditions are computed from prs_to_merge, which is captured before the loop, so a reader may wonder whether the loop can observe change at all. It can: the tail of each iteration re-queries get_status_checks for every still-pending PR and reassigns pr.check_status. The refresh is present and correct. Recording this so the next reader does not re-derive it.
2. The second early-exit is unreachable
The loop breaks when pending_count == 0, where pending_count counts PRs whose status is Pending. Further down, after the timeout check, it breaks again when all_resolved is true, where all_resolved is "no PR has status Pending".
Those two conditions are the same condition. The second break can only be true when the first already fired, so it is unreachable.
The comment above it reads "Early exit if all remaining non-passing checks have definitively failed" — a genuinely different and useful behavior, which is not what the code beneath it does. Since a definitively-failed check is not Pending, the first break already handles that case and the run proceeds to report the failure. So the behavior a user sees is correct; what is wrong is that the block adds nothing and its comment describes something else.
Worth deciding rather than deleting on sight: if the intent was to distinguish "resolved, some failed" from "resolved, all passed" and act differently, that is a real behavior worth having and the block is a stub for it. If not, the block and its comment should go.
Suggested direction
- A test that enters the loop with a check that stays pending and asserts the timeout exit is non-zero.
- A test for the pending-to-passing transition, which the new lifecycle-style stateful mocking makes straightforward: return
in_progress on the first status query and completed/success afterwards.
- A decision on item 2, then either implement the behavior the comment describes or remove both.
A test for the loop needs the timeout set to a small value so it does not sleep for the default; the sleep interval between polls is a fixed 15 seconds, which is itself worth making injectable if these tests are to run at reasonable speed.
Two findings in the
--waitpath ofgr pr merge, both small, neither a behavior defect. Filing them because the path is exercised only in the case where it does nothing.1. The polling loop is never entered by any test
--waitpolls until every PR's checks resolve, or until--timeoutelapses and the command fails.The only test naming
--waitistest_pr_merge_wait_does_not_block_when_no_checks_are_configured, which covers the case where no checks exist, soany_pendingis false and the loop is skipped entirely. Nothing enters the loop, so none of this is covered:Err(_)arm that keeps a PR pending when a status query failsConfirmed by mutation: neutering the timeout condition turns no test red.
The exit code of a timed-out wait is therefore unverified in both directions, which matters for a flag whose whole purpose is to be used unattended in scripts.
A stale-data defect was specifically ruled out. The loop's exit conditions are computed from
prs_to_merge, which is captured before the loop, so a reader may wonder whether the loop can observe change at all. It can: the tail of each iteration re-queriesget_status_checksfor every still-pending PR and reassignspr.check_status. The refresh is present and correct. Recording this so the next reader does not re-derive it.2. The second early-exit is unreachable
The loop breaks when
pending_count == 0, wherepending_countcounts PRs whose status isPending. Further down, after the timeout check, it breaks again whenall_resolvedis true, whereall_resolvedis "no PR has statusPending".Those two conditions are the same condition. The second break can only be true when the first already fired, so it is unreachable.
The comment above it reads "Early exit if all remaining non-passing checks have definitively failed" — a genuinely different and useful behavior, which is not what the code beneath it does. Since a definitively-failed check is not
Pending, the first break already handles that case and the run proceeds to report the failure. So the behavior a user sees is correct; what is wrong is that the block adds nothing and its comment describes something else.Worth deciding rather than deleting on sight: if the intent was to distinguish "resolved, some failed" from "resolved, all passed" and act differently, that is a real behavior worth having and the block is a stub for it. If not, the block and its comment should go.
Suggested direction
in_progresson the first status query andcompleted/successafterwards.A test for the loop needs the timeout set to a small value so it does not sleep for the default; the sleep interval between polls is a fixed 15 seconds, which is itself worth making injectable if these tests are to run at reasonable speed.