perf(test): reduce layer boundary fixture scans - #6322
Conversation
📝 WalkthroughWalkthroughThe layer import boundary checker now handles file and non-directory inputs before directory traversal. Its tests now call ChangesLayer Import Boundary Check
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR speeds up the integration test coverage for the layer import boundary checker by removing repeated external tsx process launches and full-tree scans, while still exercising both the “no violations” path and several negative/fixture scenarios.
Changes:
- Refactors
test/layer-import-boundaries.test.tsto callfindLayerImportBoundaryViolations()in-process and to scan only per-test synthetic fixture files for negative cases. - Extends the checker’s
walk()helper to accept a single file path as a scan root (in addition to directories), enabling fixture-only scans.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/layer-import-boundaries.test.ts |
Replaces spawnSync(tsx …) with in-process checker calls and adds fixture helpers to avoid repeated repo-wide scans. |
scripts/checks/layer-import-boundaries.ts |
Updates walk() to support file roots so tests can scan a single synthetic fixture file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/layer-import-boundaries.test.ts (1)
14-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixtures are written directly into real
src/directories.
fixturePath/scanFixturewrite and then remove fixture files under actual production directories (src/lib/domain,src/lib/messaging/manifest,src/commands) rather than a scratch/tmp location. This is likely required so the checker's path-based classification (isDomainFile,isMessagingManifestFile,isCommandFile) recognizes them, but thetry/finallycleanup won't run on a hard kill (CI timeout/OOM), leaving stray__boundary-*.tsfiles in the real source tree.Consider adding a defensive sweep (e.g. an
afterAllthat globs and removes any leftover__boundary-*files, or a.gitignore/pre-commit guard) as a safety net against this failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/layer-import-boundaries.test.ts` around lines 14 - 26, The test helpers fixturePath and scanFixture write temporary boundary files into real source directories, so a hard kill can leave stray __boundary-*.ts files behind. Add a defensive cleanup in the layer-import-boundaries test suite, such as an afterAll that removes any leftover __boundary-* fixtures created by fixturePath/scanFixture, so the classification checks still work without relying only on try/finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/layer-import-boundaries.test.ts`:
- Around line 28-90: The test titles in CLI layer import boundary coverage are
missing the required local issue-ref suffix. Update the `describe("CLI layer
import boundaries", ...)` title and every `it(...)` title in
`findLayerImportBoundaryViolations`/`scanFixture`-based cases to use
behavior-oriented wording that ends with a trailing `(`#1234`)`-style local issue
reference. Keep the test intent unchanged, just append the suffix consistently
across this file’s root-level integration tests.
---
Nitpick comments:
In `@test/layer-import-boundaries.test.ts`:
- Around line 14-26: The test helpers fixturePath and scanFixture write
temporary boundary files into real source directories, so a hard kill can leave
stray __boundary-*.ts files behind. Add a defensive cleanup in the
layer-import-boundaries test suite, such as an afterAll that removes any
leftover __boundary-* fixtures created by fixturePath/scanFixture, so the
classification checks still work without relying only on try/finally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e09bf5b5-ee97-49c9-9efd-e12c18a3e931
📒 Files selected for processing (2)
scripts/checks/layer-import-boundaries.tstest/layer-import-boundaries.test.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
9a8b4e9 to
74c7156
Compare
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 sentences: what this PR does and why. --> This PR speeds up layer import-boundary integration coverage by calling the checker in-process and scanning each synthetic negative fixture directly, while retaining one repository-wide positive scan. In the contributor's Node 22 local benchmark, the focused test file fell from 15.08s to 3.83s wall time (74.6%). ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Refs NVIDIA#6245 ## Changes <!-- Bullet list of key changes. --> - Allow the boundary checker to accept a single production TypeScript file as its scan root. - Replace five external `tsx` invocations with direct `findLayerImportBoundaryViolations()` calls. - Scope the four negative cases to their synthetic fixtures and assert structured violations directly. ## Performance Contributor-reported Node 22 measurements: | Measurement | Before | After | Improvement | | --- | ---: | ---: | ---: | | Focused test wall time | 15.08s | 3.83s | 74.6% | | Vitest duration | 13.00s | 2.70s | 79.2% | The latest reported branch run completed in 2.59s of Vitest duration, with 1.78s of test-body time. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Internal test/checker performance change with no user-facing behavior. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project integration test/layer-import-boundaries.test.ts --reporter=verbose` passed; contributor reported a 2.59s Vitest duration on Node 22. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional contributor-reported checks: `npm run checks`, `npm run build:cli`, `npm run typecheck:cli`, a focused Biome check, and `git diff --check`. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Ho Lim <subhoya@gmail.com>
Summary
This PR speeds up layer import-boundary integration coverage by calling the checker in-process and scanning each synthetic negative fixture directly, while retaining one repository-wide positive scan. In the contributor's Node 22 local benchmark, the focused test file fell from 15.08s to 3.83s wall time (74.6%).
Related Issue
Refs #6245
Changes
tsxinvocations with directfindLayerImportBoundaryViolations()calls.Performance
Contributor-reported Node 22 measurements:
The latest reported branch run completed in 2.59s of Vitest duration, with 1.78s of test-body time.
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/layer-import-boundaries.test.ts --reporter=verbosepassed; contributor reported a 2.59s Vitest duration on Node 22.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Additional contributor-reported checks:
npm run checks,npm run build:cli,npm run typecheck:cli, a focused Biome check, andgit diff --check.Signed-off-by: Ho Lim subhoya@gmail.com