Make the test selector comment more readable with reasoning - #18376
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18376Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18376" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR restructures the advisory "Tests selector" PR comment emitted by the selective-CI tool (tools/SelectTests). Previously the comment repeated the same selection reason on every project line, which became unreadable when one change fanned out across the project graph. The comment now leads with what runs (flat lists of selected jobs and test projects) and then explains how it was chosen by grouping projects under each trigger, collapsing large fan-outs into <details>, and surfacing a headline call-out when one change drives most of the selection. It also makes the comment_selection job gracefully handle the read-only GITHUB_TOKEN on fork PRs (403) by warning and writing the selection to the step summary instead of failing. Selection logic, job booleans, and one-comment-per-commit posting are unchanged.
Changes:
- Rewrote
WriteSelectionCommentto render "what runs" first, then a per-trigger rationale (AppendSelectionRationaleplus grouping/header/hops/RenderProjectListhelpers), replacing the per-line cause suffix. - Added fork-PR 403 handling in
tests.yml: warn and emit the selection into the job step summary rather than failing the informational job. - Updated CLI, Layer 1, and comment-script tests (including a large-fan-out headline/collapse test and a fork-summary test) and the design doc.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tools/SelectTests/Program.cs |
Restructured the PR comment body into "what runs" + per-trigger grouping; added grouping/header/hops/render helpers and WriteCommentFile; removed AllCausesSuffix. |
.github/workflows/tests.yml |
comment_selection now catches the fork read-only-token 403, warns with the fork cause, and writes the selection to the step summary. |
tests/Infrastructure.Tests/WorkflowScripts/SelectTestsCommentScriptTests.cs |
Added a fork-403 test asserting no comment/minimize, a warning, and a step-summary fallback containing the selection. |
tests/Infrastructure.Tests/WorkflowScripts/select-tests-comment.harness.js |
Mock harness now captures warnings/summary and can simulate a createComment 403. |
tests/Infrastructure.Tests/TestTriggerMap/SelectTestsLayer1IntegrationTests.cs |
Updated assertions for the new grouped graph rendering. |
tests/Infrastructure.Tests/TestTriggerMap/SelectTestsCliTests.cs |
Updated assertions to the new headings; added a large-fan-out headline/<details> collapse test. |
docs/ci/test-trigger-selector-design.md |
Documented the new comment structure and the fork step-summary fallback. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
fbfeb4d to
3b6addd
Compare
This comment has been minimized.
This comment has been minimized.
The selective-CI "Tests selector" comment from tools/SelectTests listed every selected test project on its own line with the reason appended, so a single change fanning out across the project graph repeated the same reason on dozens of lines. A reviewer couldn't see at a glance what runs or which change drove the bulk of the selection. Restructure the rendered comment to lead with what runs -- a flat list of selected jobs and a flat list of selected test projects -- then explain how it was chosen by grouping the selected projects under each trigger (a changed file and its graph fan-out under one heading, an affected project, or a derived test). Each reason is stated once instead of per project, large fan-outs collapse into a <details> block, a per-job table names what triggered each job, and a headline call-out flags the single change driving a large share of a big selection. Full attribution is preserved: a project selected by several triggers appears under each group. No change to the selection logic, job booleans, or one-comment-per-commit posting behavior -- only the comment body layout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the comment-rendering tests for the new grouped layout: assert the "what runs" job/project lists, the per-trigger grouping with every cause preserved, and the per-job reasons table. Add LargeFanOutEmitsHeadlineAnd- CollapsesProjectList to cover the headline call-out plus the <details> collapse threshold for big fan-outs, and MultiHopGraphSelectionAnnotates- HopCountInComment to verify the hop-count annotation on a multi-hop graph path (via a GraphRepoFixture that can insert an intermediate project). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the test-trigger selector design doc to describe the reworked PR comment: it leads with the flat job/project lists, then groups the selection under each trigger (with a changed file's graph fan-out collapsed under one heading), keeps full per-trigger attribution, and adds a per-job reasons table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3b6addd to
360f70c
Compare
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 1 / 97 test projects · 0 jobs, from 3 changed files. Selected test projects (1 / 97)
Selected jobs (0)none How these were chosen — grouped by what changed🧪 🧪 📄 Job reasonsnone Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
JamesNK
left a comment
There was a problem hiding this comment.
Clean implementation. The trigger-group inversion, threshold gating, nested <details> rendering, and hop annotations all look correct. Tests directly target the failure modes (revert detection for ordering, collapse, headline, and job-reason table). No issues found.
|
✅ No documentation update needed. docs_optional → build_or_ci_only No documentation PR is needed for this change. Triggered signals: none (signal_count = 0; Allowlist rationale —
The change restructures the format of the selective-CI PR comment posted by |
What this changes
The Tests selector PR comment posted by
tools/SelectTests(theselective-CI advisory comment) was hard to read. It listed every selected
test project on its own line with the selection reason appended, so when one
change fans out across the project graph the same reason repeats on dozens
of lines. A reviewer couldn't tell at a glance which projects and jobs run,
or which change drove the bulk of the selection.
Example — the old comment had 46 near-identical lines like:
The fix
Restructure the comment into what runs, then how it was chosen:
flat list of selected jobs. Full impact is visible up front, even with many
changed files.
<details>block (the heading is its
<summary>), so the comment opens on the listsabove and a reviewer expands only when they want the rationale. Inside,
the selected projects are grouped under each trigger (a changed file and
its graph fan-out under one heading, an affected project, or a derived
test). Each reason is stated once instead of per project; large fan-outs
collapse into a nested
<details>; a per-job table names what triggeredeach job. A headline
⚠️call-out names the single change driving a largeshare of a big selection.
Call-outs
appears under each group, so no "why" is dropped.
and collapse paths) and
docs/ci/test-trigger-selector-design.md.posting behavior — only the comment body layout.
Supersedes #18368 (which was opened from a fork, so its selection comment
couldn't post).