test(e2e): enforce exact partition in assertGenerateMatrixCoversTargets and correct overstated coverage comments#2124
Merged
Conversation
…ts and correct overstated coverage comments Follow-ups from the PR #2116 review. Finding 1: assertGenerateMatrixCoversTargets documented an "exact partition" of the declared targets into `testedTargets` and `untested`, but only checked `stray` and `uncovered`. A target left in both `testedTargets` and `untested` (or duplicated within `testedTargets`) broke the exclusive-partition invariant while CI stayed green. Add explicit duplicate and overlap checks so those cases fail CI. Finding 2: soften the in-spec comments for the permissions generate list and the hooks project/global standalone lists to state accurately that the completeness check only enforces enumeration == processor-declared set, not that a matching `it` body exists for each name. Closes #2120 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review follow-ups on the exact-partition enforcement: - Symmetrically detect duplicates within `untested`, not just `testedTargets`, since a true exact partition requires each side to be internally unique. - Extract the sorted-unique-duplicates logic into a module-scope helper so the duplicate error messages show each name once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…de duplicates The earlier duplicate-within-`testedTargets` check was too strict: a tool can legitimately appear more than once in `testedTargets` when it is exercised by several matrices (e.g. antigravity-ide emits both a root AGENTS.md and a nested .agents/rules tree, so `e2e-rules.spec.ts` lists it in both matrix arrays). That is not a contradiction and must not fail CI. The only real invariant the stray/uncovered checks cannot catch is a tool listed in BOTH `testedTargets` and `untested` (each set covers it, so neither fires). Keep just that disjointness (overlap) check and document that within-side duplicates are allowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
Merged
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
2 participants
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.
Background
Follow-ups from the review of PR #2116 ("test(e2e): assert the generate matrix covers every declared tool target"). Two non-blocking (severity: low) findings were captured in #2120 so they would not be lost.
Changes
Finding 1 — enforce the exact partition
assertGenerateMatrixCoversTargetsdocumented that declared targets "partition exactly" intotestedTargetsanduntested, but only checkedstray(declared-outside) anduncovered(not-covered). A target left in bothtestedTargetsanduntested— or duplicated withintestedTargets— broke the exclusive-partition invariant while CI stayed green.Added an explicit duplicate check and an overlap (intersection) check so those cases now fail CI, matching the documented contract.
Finding 2 — correct overstated coverage comments
For the permissions generate list and the hooks project/global standalone lists, each tool has its own hand-written
it, so the completeness check only verifies "enumeration == processor-declared set" — not that a matchingitbody exists for each name. Softened those in-spec comments so they no longer overstate the guarantee (deleting a tool'sitwhile leaving its name in the list would keep the check green).Verification
pnpm cicheckpasses (all 6752 tests green, format/lint/typecheck clean).Closes #2120
🤖 Generated with Claude Code