Skip to content

test(lint): tighten seam regex + drop reference-only suites (#698)#707

Merged
malpern merged 1 commit into
masterfrom
fix/698-io-mocking
Jun 1, 2026
Merged

test(lint): tighten seam regex + drop reference-only suites (#698)#707
malpern merged 1 commit into
masterfrom
fix/698-io-mocking

Conversation

@malpern
Copy link
Copy Markdown
Owner

@malpern malpern commented Jun 1, 2026

Part of #698 — shrinks the test-seam allowlist 8 → 4.

  1. Tighten the regex with a leading word boundary (\b) so it stops matching a method name ending in a type (e.g. testDoNotBypassInstallerEngine() was a false positive flagging FacadeLintTests).
  2. Drop 4 reference-only suites that only name the types but never construct them: FacadeLintTests, PrivilegedOperationsCoordinatorTests, SystemRequirementsTests, WizardPureLogicTests.

Remaining allowlist (4) is the genuinely-hard set: ErrorHandlingTests, KeyPathTests, RuntimeCoordinatorResetTests (real config-regeneration I/O) + VHIDDeviceManagerTests (the seam's own test).

Verified swift test --filter TestSeamLintTests passes in 0.08s.

Two cleanups to the TestSeamLintTests ratchet:

1. Anchor the hazard-type regex with a leading word boundary (\b). Without it,
   the pattern matched a method NAME ending in a type, e.g.
   `testAppKitSourcesDoNotBypassInstallerEngine()`, flagging FacadeLintTests as a
   false positive.

2. Drop 4 reference-only suites from the allowlist (they only name the types, never
   construct them, so the tightened regex no longer flags them): FacadeLintTests,
   PrivilegedOperationsCoordinatorTests, SystemRequirementsTests, WizardPureLogicTests.

Allowlist shrinks 8 → 4. The remaining 4 are the genuinely-hard cases:
ErrorHandlingTests, KeyPathTests, RuntimeCoordinatorResetTests (real saveConfiguration/
resetToDefaultConfig I/O — see #698) and VHIDDeviceManagerTests (the seam's own test).

Verified: `swift test --filter TestSeamLintTests` passes in 0.08s.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Code Review — PR #707: Tighten seam regex + drop reference-only suites

Overview

This PR does two tightly-coupled things to shrink the test-seam allowlist from 8 → 4:

  1. Adds a leading \b word boundary to the hazard-type constructor pattern so method names ending in a type name (e.g. testAppKitSourcesDoNotBypassInstallerEngine()) are no longer false positives.
  2. Drops 4 suites from the allowlist that were only on it because of that false positive or because they never actually construct a hazard type.

Correctness: ✅

Regex fix is sound. The old pattern (RuntimeCoordinator|InstallerEngine|...)\s*\( matched anywhere those strings appear before a (, including inside a method name. Adding \b before the group means the type name must start on a word boundary — so testDoNotBypassInstallerEngine( is no longer matched. I verified with grep -P that the new regex produces no match in all 4 dropped files.

Dropped files are correctly excluded. Breaking down each:

  • FacadeLintTests.swift — The documented false positive. Old regex matched testAppKitSourcesDoNotBypassInstallerEngine(). New regex with \b correctly ignores it. ✅
  • SystemRequirementsTests.swift — References SystemValidator only in a comment. Neither old nor new regex would match a constructor or type annotation. ✅
  • WizardPureLogicTests.swift — References .activateVHIDDeviceManager (an enum case). No constructor or annotation match. ✅
  • PrivilegedOperationsCoordinatorTests.swift — Uses VHIDDeviceManager.testPIDProvider = { ["123"] } (setting the seam's test hook). No VHIDDeviceManager( constructor, so neither regex would flag it. ✅

One minor inaccuracy in the PR description: It says all 4 dropped suites "only name the types but never construct them." For PrivilegedOperationsCoordinatorTests.swift this undersells what it does — it actively uses the seam by setting testPIDProvider. That said, the end result is correct: the file never constructs a hazard type, so it shouldn't be on the allowlist regardless.

The Ratchet Invariant: ✅

Allowlist shrinks from 8 → 4. The Never add new entries doc comment is preserved. Remaining 4 entries have clear explanatory comments for why they're still there (real I/O vs. the seam's own tests). The invariant holds.

Minor Observations

Second alternative doesn't need \b at the start — it's already correct. The pattern :\s*(TypeName)\b can only fire on : TypeName (type annotation), not on arbitrary tokens. The trailing \b already prevents partial-word matches. No change needed.

File-name-based allowlist caveat (pre-existing, not introduced here): The allowlist matches on lastPathComponent only. Two files in different directories with the same name would share an allowlist slot. All current entries are unique, so this isn't a problem now, but worth noting if new entries are ever added.

Tests

PR claims swift test --filter TestSeamLintTests passes in 0.08s. The test is self-verifying (it would catch a regression in the regex or an incorrectly-dropped entry). No additional test coverage is needed for a change of this scope.


Verdict: LGTM. The regex fix is correct, all 4 dropped files are verified to be safe to remove, and the allowlist ratchet is maintained. The minor description inaccuracy is not blocking.

@malpern malpern merged commit 94c7525 into master Jun 1, 2026
3 checks passed
@malpern malpern deleted the fix/698-io-mocking branch June 1, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant