Deflake release-readiness recency test: remove wall-clock-dependent live assertions, add deterministic fixture coverage - #36004
Conversation
…ive assertions, add deterministic fixture coverage The release-readiness skill's own test suite contained a time-bomb: four live, wall-clock-dependent assertions hardcoded hasRecentActivity = $true against real release branches (SR8, SR9, the active-SR foreach loop, and preview6). hasRecentActivity is computed by the detector as `git log <branch> --since=7.days` > 0, so it is only true while the real branch has had a commit in the last 7 days. The moment a servicing branch goes quiet for 7 days — a NORMAL end-of-cycle state — the assertion flips red. On 2026-06-18 this happened: SR8's last commit (the SR7->SR8 merge #35810) landed 2026-06-11, so the 7-day window returned 0 and the two SR8 assertions went red. They self-heal on the next commit, but that is a credibility bug in a suite whose entire value proposition is determinism. This change: - Removes the wall-clock-dependent hardcoded-$true live assertions. The end-to-end detector run now asserts only that hasRecentActivity is a real [bool] the detector emitted, never a date-dependent value. - Adds genuinely deterministic coverage of the recency-window math via a synthetic fixture: a throwaway temp git repo with commits at controlled dates (GIT_AUTHOR_DATE/GIT_COMMITTER_DATE), then calls the REAL Get-RecentCommitCount (dot-sourced) against it and asserts exact counts for 7/10/60/1-day windows plus the origin/ ref form. Zero network, zero dependence on "today"; the temp repo is cleaned up in a finally. - Corrects the misleading comments that equated "active SR" with hasRecentActivity = true; an active SR can legitimately idle >7 days. Follow-up to #35971. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36004Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36004" |
Adversarial review round-1 hardening of the synthetic recency fixture: - Force commit.gpgsign=false locally so the throwaway repo builds on dev machines that enable commit signing globally (no key -> 0 commits). - Add a precondition guard asserting 3 commits on origin/main, so a swallowed git misconfig fails loudly instead of as a cryptic 'unknown revision' later. - Add -ErrorAction SilentlyContinue to the finally-block cleanup so a cleanup hiccup can't mask a real failure from the try body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adversarial review round-2 consensus fixes: - Mapping coverage (2/3): the live assertions only checked hasRecentActivity is a [bool]; the detector's actual wiring (hasRecentActivity = recentCommitCount > 0) was unasserted, so an inverted/hardcoded mapping could ship silently. Add a date-INDEPENDENT invariant on the SR and preview trackers: hasRecentActivity -eq (recentCommitCount > 0). Both fields come off the same tracker at the same instant, so it never flakes yet still trips on a broken mapping. (+3 assertions: SR8, SR9, preview6.) - Hermetic hooks (2/3): a global core.hooksPath or an init.templateDir that seeds .git/hooks could install a pre-commit hook that rejects the synthetic commits. Redirect hook lookup to an empty path under .git (overrides global hooksPath and bypasses templated hooks). - Env restore (1/3): save/restore ambient GIT_AUTHOR_DATE/GIT_COMMITTER_DATE around the commit loop instead of blindly Remove-Item-ing them, so a caller that pre-set those vars isn't left mutated. Full suite: 555/0 (E2E), 489/0 (-SkipE2E). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
Note
🤖 This review was automatically generated by a multi-model AI review system (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro). Three models independently reviewed the code, then cross-pollinated their findings to produce this consolidated review.
Multi-Model Review — PR #36004 Round 1
Verdict: ✅ LGTM (Unanimous)
Confidence: High
Summary
Perfect unanimous approval. This PR correctly deflakes a pre-existing time-bomb in the release-readiness skill's test suite by replacing wall-clock-dependent assertions with deterministic synthetic fixtures.
What This PR Fixes
The Problem: Four live assertions hardcoded hasRecentActivity = $true against real branches (SR8, SR9, active-SR loop, preview6). These assertions were only true while the branch had a commit in the trailing 7 days. When SR8 went quiet for 7 days (normal end-of-cycle state), two assertions flipped red on 2026-06-18.
The Solution:
- Removed wall-clock-dependent live assertions — Now asserts only that
hasRecentActivityis a real[bool], never a date-dependent value - Added deterministic synthetic coverage — Throwaway temp git repo with commits at controlled dates (
GIT_AUTHOR_DATE/GIT_COMMITTER_DATEat now−6d / now−8d / now−30d) - Tests real code — Calls the actual
Get-RecentCommitCounthelper (dot-sourced) and asserts exact counts for 7/10/60/1-day windows - Proper cleanup —
try/finallycleans up both temp repo and mutated environment variables
Verification (All 3 Reviewers Agreed)
✅ Time-bombs removed — No more date-dependent assertions on live branches
✅ Deterministic fixture — Controlled commit dates, zero network, zero wall-clock dependence
✅ Complete coverage — All four recency windows (7/10/60/1-day) tested
✅ Proper cleanup — Temp repo and environment cleaned in finally block
✅ Suite results — Passed: 552, Failed: 0 (was 545/2 before with SR8 reds)
✅ Offline pass — New fixture assertions pass under -SkipE2E, confirming network-independence
Cross-Pollination Notes
- All 3 models: Independent LGTM with high confidence
- Opus: Verified fixture uses real
Get-RecentCommitCount, proper cleanup, hermetic coverage - GPT: Confirmed time-bomb removal and deterministic replacement
- Gemini: Validated synthetic fixture quality and safer E2E invariant
- Zero disagreements — Perfect alignment across all reviewers
This is a clean, low-risk deflaking fix that makes the test suite reliable and maintainable. Ready for merge.
The SR readiness report derived blocking only from ship-checks and Tier-1 regression issues; open PRs were informational. A p/0-labelled PR targeting an SR branch (e.g. #35970 against release/10.0.1xx-sr8) was therefore not flagged as blocking on the SR tracker issue, unlike the Preview lane which already treats p/0 PRs as blockers. Port Test-IsP0Pr into Get-ReleaseReadiness.ps1 and add a Get-P0PrChecks function that emits a 'P/0 release-branch PRs' ship-check (BLOCKED when any open p/0 PR targets the SR branch, READY otherwise). The check reuses the already-fetched open-PR list (no extra gh call) and is merged into shipChecks, so a BLOCKED result is auto-hoisted into the top-of-issue blocking summary and escalates the verdict to Not Ready via the existing ship-check machinery. Add deterministic synthetic-fixture unit tests for both functions. Discovered while working on deflake PR #36004. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The SR readiness report derived blocking only from ship-checks and Tier-1 regression issues; open PRs were informational. A p/0-labelled PR targeting an SR branch (e.g. #35970 against release/10.0.1xx-sr8) was therefore not flagged as blocking on the SR tracker issue, unlike the Preview lane which already treats p/0 PRs as blockers. Port Test-IsP0Pr into Get-ReleaseReadiness.ps1 and add a Get-P0PrChecks function that emits a 'P/0 release-branch PRs' ship-check (BLOCKED when any open p/0 PR targets the SR branch, READY otherwise). The check reuses the already-fetched open-PR list (no extra gh call) and is merged into shipChecks, so a BLOCKED result is auto-hoisted into the top-of-issue blocking summary and escalates the verdict to Not Ready via the existing ship-check machinery. Add deterministic synthetic-fixture unit tests for both functions. Discovered while working on deflake PR #36004. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…R lane parity with Preview) (#36006) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### The gap The release-readiness skill has two report generators. The **Preview lane** (`Get-PreviewReadiness.ps1`) already surfaces open PRs carrying the `p/0` label as release **blockers**. The **SR lane** (`Get-ReleaseReadiness.ps1`) did **not**: it derived blocking only from ship-checks (`BLOCKED`) and Tier-1 `regressed-in-*` **issue** classifications. Open PRs were listed purely informationally. Consequence: a `p/0`-labelled PR targeting an SR branch was never flagged as blocking on the SR tracker issue. Concretely, PR #35970 (`Revert PR #33584…`, base `release/10.0.1xx-sr8`, label `p/0`) did **not** show as blocking on the SR8 tracker issue #35876. ### What changed - **Ported `Test-IsP0Pr`** from the Preview lane into `Get-ReleaseReadiness.ps1`. It is StrictMode-safe and accepts both the production `gh --json` PSCustomObject shape and the IDictionary/hashtable shape used by test mocks. - **Added `Get-P0PrChecks`**, which emits a single `P/0 release-branch PRs` ship-check — `BLOCKED` (naming each offending PR, e.g. `#35970`) when any open `p/0` PR targets the SR branch, `READY` otherwise. - **Merged the check into `shipChecks`** in the main flow, reusing the already-fetched open-PR list so there is **no extra `gh` call**. Because it's a standard `BLOCKED` ship-check, it is automatically hoisted into the top-of-issue `🔴 Blocking` summary and escalates the verdict to **Not Ready** — no verdict or renderer changes were needed. - **Added deterministic synthetic-fixture unit tests** for both functions (no network, no wall-clock dependence). ### Notes - This is a PowerShell-only `.github/skills/**` change; the framework `maui-pr` pipeline intentionally skips for skills-only PRs. - Discovered while working on the separate deflake PR #36004. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Summary
Follow-up to #35971. De-flakes a pre-existing time-bomb in the release-readiness skill's own test suite (
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1). Skills-only change — no framework code touched.The time-bomb
The suite had four live, wall-clock-dependent assertions that hardcoded
hasRecentActivity = $trueagainst real release branches (SR8, SR9, the active-SRforeachloop, and preview6).The detector (
Find-ReleaseReadinessTrackers.ps1) computeshasRecentActivityfromGet-RecentCommitCount, which runsgit log <branch> --since=7.daysand returns(count > 0). So those assertions are only true while the real branch has had a commit in the last 7 days. The moment a servicing branch goes quiet for 7 days — a normal end-of-cycle state — the assertion flips red.On 2026-06-18 this actually happened: SR8's last commit (the SR7→SR8 merge #35810) landed 2026-06-11, so
--since=7.daysreturned 0 and the two SR8 assertions went red. They self-heal on the next commit, but nondeterministic red in a suite whose entire selling point is determinism is a credibility bug.What changed
hasRecentActivityis a real[bool]the detector emitted — never a date-dependent value.GIT_AUTHOR_DATE/GIT_COMMITTER_DATEat now−6d / now−8d / now−30d), then calls the realGet-RecentCommitCount(dot-sourced) against it and asserts exact counts for the 7/10/60/1-day windows plus theorigin/-prefixed ref form. Zero network, zero dependence on "today"; the temp repo is cleaned up in afinally.hasRecentActivity = true. An active SR can legitimately idle for >7 days;hasRecentActivityis a 7-day-window signal, not a synonym for "active".Verification
Full suite is green:
pwsh -NoProfile -File .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1→ Passed: 552, Failed: 0 (was 545/2 before, with the two SR8 reds). The new fixture assertions also pass under-SkipE2E(offline), confirming they're network-independent.