You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Six unique findings from the Copilot reviewer (each posted twice across
two review passes):
1-3. Missing .lock.yml files for code-review, evaluate-pr-tests, learn-from-pr
→ Compiled via 'gh aw compile'. Lock files + .github/aw/actions-lock.json
delta now included.
4-5. evaluate-pr-tests.md gate exits 1 for legitimate skip cases (PR not OPEN;
no test files in diff), causing the workflow run to show as failed.
→ Changed to 'exit 0' with ::notice:: emission. The downstream agent
still activates and calls 'noop' when there's no work — one wasted
activation per skip is acceptable cost to avoid false CI failures.
Genuine infra failures (gh API errors) still exit 1.
6. validate-pat-pool.yml summary text said empty pool would 'fall back to
COPILOT_GITHUB_TOKEN' but the workflow exits 1 on empty pool.
→ Reworded the empty-pool summary to acknowledge both behaviors:
runtime falls back (correctly), but the validator surfaces empty as a
failure to prompt operators to seed the pool. Kept exit 1 — alerting
on empty pool is the desired behavior.
Additional fixes uncovered by compilation:
- learn-from-pr.md: dropped 'imports: shared/pat_pool.md' and the engine
PAT-pool case() expression. The shared pat_pool job needs a pre_activation
job that workflow_dispatch-only triggers don't auto-generate. learn-from-pr
is manually invoked, so default COPILOT_GITHUB_TOKEN is fine (same pattern
as cswin32-update.md).
- evaluate-pr-tests.md: changed workflow_dispatch input pr_number from
'required: true' to 'required: false' — slash_command triggers can't
enforce required manual inputs (gh aw compile rejects this combination).
- evaluate-pr-tests.md: removed 'bots: copilot-swe-agent[bot]' — gh aw warns
that bots + slash_command can cause a bot's comment starting with the
command to occupy the concurrency slot, blocking manual invocations.
SKILL.md improvements (informed by simulating the workflow against real
recently-opened PRs):
- evaluate-pr-tests/SKILL.md: added dimension D0 (Substantive value), weighted
highest. The rubric was rewarding tests that score well on style/convention
even when they duplicated pre-existing coverage or asserted non-existent
invariants. D0 leads the evaluation with 'what does this test exercise that
wasn't covered before, and does the assertion match the source?'
- Added an 'Assertion fabrication' check explicitly: agents must trace
assertions back to the production source line(s) and verify the relationship
before scoring D4 ✅. Documents four common failure modes (cargo-cult
invariant, convenient enumeration, tautology, self-referential setup) at
severity 🚩 above ⚠️.
- Added 'Coverage-burst pattern recognition' for multi-author 'improve test
coverage' waves common with new-contributor onboarding bursts: extra
scrutiny to D0, look for 'ISSUE #N FIX' comments, don't let headline test
counts overstate real coverage gain, calibrate tone direct rather than
encouraging.
- Added dimension D9 (Consolidation opportunities) — recognize when 5+
near-identical [Fact] tests differing only in input value should be one
[Theory] with [InlineData] rows. Surfaced as the headline finding on a real
simulated PR (PR #67019, InputDate) where 14 tests collapse cleanly to 2.
- Step 1 now explicitly requires fetching the pre-existing version of each
changed test file (via merge-base) for duplicate detection, and fetching
the production source for assertion-fabrication checks.
Copy file name to clipboardExpand all lines: .github/skills/evaluate-pr-tests/SKILL.md
+32-4Lines changed: 32 additions & 4 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -21,31 +21,59 @@ For each test added or modified in the PR, assess against the rubric below. Scor
21
21
22
22
### Rubric
23
23
24
+
> **Lead with substantive value, not rubric compliance.** A test that scores ✅ on every dimension below but duplicates pre-existing coverage or asserts a non-existent invariant adds *negative* value to the codebase. Always answer "what does this test exercise that wasn't covered before, and does its assertion match what the production code actually does?" before scoring style/convention dimensions.
|**D1: Does the test exercise the change?**| Test passes identically with the change reverted | Test fails without the change, but only on one obvious path | Test fails without the change, exercises the changed condition specifically |
28
+
|**D0: Substantive value (apply first, weighted highest)**| Test duplicates pre-existing coverage; or asserts behavior the production code doesn't actually implement; or asserts a tautology (literal constant, type identity) | Test covers a real path but the marginal coverage over existing tests is minimal | Test covers a previously-uncovered branch, parameter combination, or interaction; assertion matches what the source actually does |
29
+
|**D1: Does the test exercise the change?** (bug-fix PRs only) | Test passes identically with the change reverted | Test fails without the change, but only on one obvious path | Test fails without the change, exercises the changed condition specifically |
27
30
|**D2: Is the right test framework used?**| xUnit v2 / NUnit / MSTest used where v3 is the convention; component unit logic using a browser-based framework | E2E test used for what's clearly a unit-level scenario | Component unit logic → **TestRenderer pattern** (shared infra from `src/Components/Shared/test/`); E2E → **Selenium** under existing `src/Components/test/E2ETest/`, or **Playwright** for new E2E projects (preferred for greenfield); general unit → xUnit v3 |
28
31
|**D3: Are edge cases covered?**| Only the happy path | Happy path + one trivial error case | Boundaries (null, empty, max), error paths, concurrent access where relevant |
29
32
|**D4: Are assertions specific?**|`Assert.NotNull(result)` only; tautological | Some specific assertions, mostly type-checks | Semantically meaningful — values, behavior, side-effects on collaborators |
30
33
|**D5: Is test isolation correct?**| Shared mutable state between tests; ordering-dependent | Some shared fixtures used inappropriately | Each test sets up its own state; uses `IClassFixture<T>` / `ICollectionFixture<T>` correctly |
31
34
|**D6: Does the test reflect aspnetcore conventions?**| "Arrange/Act/Assert" comments present; uses `== null`; explicit Moq.Setup chains that should use newer APIs | Mixed | Follows `.github/copilot-instructions.md` and nearby file style |
32
35
|**D7: For Blazor PRs — render mode coverage**| Component supports multiple render modes but no E2E coverage exercises both | Only one render mode covered when the change is render-mode-sensitive | E2E tests under `src/Components/test/E2ETest/` cover the relevant render modes (TestRenderer unit tests do not model render modes — render-mode parity must be proven at the E2E level) |
33
36
|**D8: For Blazor PRs — pre-rendering**| Pre-render path not tested when component fetches data | Pre-render tested but `PersistentComponentState` not verified | Pre-render → interactive transition covered with no double-fetch |
37
+
|**D9: Consolidation opportunities**| 5+ near-identical `[Fact]` tests differing only in input value | 2-4 `[Fact]` tests that could be one `[Theory]`| Appropriate use of `[Theory]`/`[InlineData]` for parameter sweeps; distinct `[Fact]` only for distinct scenarios |
34
38
35
39
### Severity rules
36
40
37
-
-**Any ❌ on D1** (test doesn't exercise the change) → overall verdict is `❌ Tests are insufficient`.
41
+
-**Any ❌ on D0** (substantive value) → overall verdict is `❌ Tests are insufficient`. A duplicate or wrong-invariant test makes the PR worse, not better.
42
+
-**Any ❌ on D1** (test doesn't exercise the change, for bug-fix PRs) → `❌ Tests are insufficient`.
38
43
-**Multiple ⚠️** without ❌ → `⚠️ Tests need improvement`.
39
44
-**All dimensions ✅ or N/A** → `✅ Tests are adequate`.
40
45
46
+
### Assertion fabrication — the most important check
47
+
48
+
Before scoring any test ✅ on D4, **read the production code the test claims to exercise** and verify the assertion describes what the source actually does. Common failure modes:
49
+
50
+
1.**Cargo-cult invariant**: assertion locks in a behavior from a different framework or system (e.g., "MVC behavior") that the Blazor/aspnetcore code doesn't implement. The test passes only because of how the test enumerates state, not because of the framework's behavior.
51
+
2.**Convenient enumeration**: the assertion iterates "the first N items" or "until the next sentinel" and stops, which makes the assertion trivially true regardless of the broader behavior.
52
+
3.**Tautology**: asserts a literal constant the source code writes verbatim (e.g., `Assert.Equal("radio", typeAttribute)` for a line that reads `builder.AddAttribute(3, "type", "radio")`). Passes when the source compiles; provides no regression coverage.
53
+
4.**Self-referential setup**: test arranges state that makes its own assertion trivially true, with no observable behavior under test.
54
+
55
+
When you suspect any of these, **trace the assertion back to the specific source line(s)** it's supposed to cover and verify the relationship. Flag suspected cases at severity 🚩 (above ⚠️) with explicit reasoning — these are the highest-priority feedback for the author.
56
+
57
+
### Coverage-burst pattern recognition
58
+
59
+
When evaluating a PR that's part of a multi-author "improve test coverage" wave (common pattern: 3+ similar PRs from new contributors over a short window targeting adjacent components), apply extra scrutiny to D0 (substantive value):
60
+
61
+
-**Compare new tests against pre-existing tests in the same file** — duplicate detection is the most common issue.
62
+
-**Look for "ISSUE #N FIX" comments in test bodies** — signal of iterative AI-assisted authoring that may have over-generated tests.
63
+
-**Check whether the headline test count overstates the real coverage gain** — "15 new tests" is meaningless if 6 are duplicates and 1 is wrong.
64
+
-**Don't soften feedback to be encouraging** — a PR that adds 8 valuable tests and 4 duplicates is more useful than one that adds 12 mixed-quality tests; the contributor's *next* PR will be better if this one's feedback is direct.
65
+
41
66
## Process
42
67
43
68
### Step 1: Gather context
44
69
45
70
1. Fetch the PR diff and the list of changed test files.
46
71
2. For each test file: read the **full file** (not just the diff). You need the surrounding test class context.
47
-
3. For each non-test source file changed: identify what behavior changed (this is what the tests must exercise).
48
-
4. Note the test project type: TestRenderer-based unit tests (pulling in `$(ComponentsSharedSourceRoot)test\**\*.cs`), plain xUnit, Selenium-based E2E (`src/Components/test/E2ETest/`), Playwright (`src/ProjectTemplates/test/Templates.Blazor.Tests/` today; preferred for new E2E projects), etc.
72
+
3.**Fetch the production source the tests claim to exercise** — typically the `src/` peer of the changed `test/` file. This is what D0 (substantive value) and the assertion-fabrication check require.
73
+
4.**Fetch the pre-existing version of each changed test file** (`git show <merge-base>:<path>`) and enumerate what tests already covered. Any new test that overlaps an existing one is a duplicate-detection candidate.
74
+
5. For each non-test source file changed: identify what behavior changed (this is what the tests must exercise).
75
+
6. Note the test project type: TestRenderer-based unit tests (pulling in `$(ComponentsSharedSourceRoot)test\**\*.cs`), plain xUnit, Selenium-based E2E (`src/Components/test/E2ETest/`), Playwright (`src/ProjectTemplates/test/Templates.Blazor.Tests/` today; preferred for new E2E projects), etc.
76
+
7. If the PR title or labels suggest it's part of a coverage burst (multi-author test-coverage wave), apply the extra-scrutiny rules above to D0.
0 commit comments