2 Commits

Author	SHA1	Message	Date
Ignacy Łątka	82db9e2f9f	test: stop tests leaving files in the shared system temp directory (#1057) ... Three issues, one theme: tests and the shared system temp directory. - **#858 / #854** were live leaks — three call sites captured a real temp path and never removed it. One reaches the cleanup its file already had; the other two get a cleanup the file lacked. - **#836** turned out to be **already fixed on `main`** by #714, which scoped those three tests' `TMPDIR` to a directory they own. This PR verifies that rather than changing it — reproduction and mutation evidence below. - Sweeping the same shape across the two packages turned up **six more test files** doing it, worth **44** leftover entries per run. - A third shape, in **two** more files: `resolveFileInputs` hands its caller a `cleanup` for whatever it materialized — the dispatcher's job in production. Five call sites kept the result and never called it, stranding 5 dirs per run. A scoped-`TMPDIR` run of all 12 affected files leaves **one** entry behind: an empty `argent-chromium-media/`, production's own fixed staging dir. 173 tests, no other file or directory. Fixes #858 Fixes #854 Closes #836 <details> <summary>#858 — before/after temp scope</summary> ``` rm -rf /tmp/scope && mkdir -p /tmp/scope TMPDIR=/tmp/scope TMP=/tmp/scope TEMP=/tmp/scope \ npx vitest run --root packages/tool-server test/chromium-server-screenshot.test.ts ``` | site | before | after | |---|---|---| | `chromium-server-screenshot.test.ts` | `argent-chromium-media/argent-screenshot-test-1788803062089-38318.png` | *(no PNG)* | | `chromium-cdp-blueprint.test.ts` | `argent-chromium-media/argent-screenshot-chromium-cdp-52204-1788803091335-39401.png` | *(no PNG)* | Both files keep an empty `argent-chromium-media/` — that is production's fixed staging dir (`src/chromium-server/cdp-session.ts:218`), shared with any live chromium-server on the machine and not per-run garbage. A test `rm`ing it would be the machine-global-resource defect #714 removed elsewhere, so it is left alone deliberately. </details> <details> <summary>#854 — before/after temp scope</summary> ``` TMPDIR=/tmp/scope … npx vitest run --root packages/argent-mcp test/content.test.ts ``` Before (`34 passed`, two directories left): ``` argent-mcp-content-kPfgYa/context.diff.png argent-artifacts/test-temp-dir-hygiene-037c31/20260907-174453-39458/home-diff.png ``` After: **empty**. The `mkdtemp` moved onto a `beforeEach`/`afterEach` pair mirroring the sibling describe, and `flowRunToMcpContent` now pins `ARGENT_ARTIFACTS_DIR` the way the artifact describe above it already did. The unconditional `delete process.env.ARGENT_ARTIFACTS_DIR` is kept, not converted to a save/restore: it is what keeps any future unpinned write landing in `tmpdir` rather than a developer's configured artifacts directory. The pin is not hygiene-only — it is asserted. Removing `process.env.ARGENT_ARTIFACTS_DIR = root` fails the test: ``` - diff: /tmp/scopeM4/content-flow-artifacts-LZKPAl + diff: /tmp/scopeM4/argent-artifacts/test-temp-dir-hygiene-037c31/20260908-100837-89701/home-diff.png ❯ test/content.test.ts:627 expect(artifactText?.text).toContain(`diff: ${root}`) ``` </details> <details> <summary>#836 — why no code changed, and the evidence</summary> (This PR does change `file-inputs.test.ts`, but for the unrelated third shape above — not for the sites #836 cites.) The issue cites `readdir(os.tmpdir())` at `file-inputs.test.ts:180-197` and `flow-visual.test.ts:586-602`. On current `main` neither exists: #714 replaced both (and the `http-upload.test.ts:154-158` sibling the issue also flags) with a per-test scratch directory the test creates, points `os.tmpdir()` at via `redirectTmpdir`, and removes. There is now **no** `readdir` of `os.tmpdir()` anywhere in either package. The per-path assertion the issue asks for is not reachable at these three sites — every name is a `mkdtemp` random suffix or a server-side `randomUUID`, so the test cannot know the path. Scoping the tmpdir to a directory the test owns is the same guarantee, and is what landed. **Reproduced the reported failure** by reverting each site to the global-listing shape, then running two simultaneous instances in a loop: | file | pre-#714 shape | current | |---|---|---| | `file-inputs.test.ts` | **1 / 12** — `expected [ 'argent-file-input-00mPU7', …(1371) ] to deeply equal [ …(1370) ]` | **0 / 12** | | `flows/flow-visual.test.ts` | **12 / 12** — `expected [ 'argent-flow-crop-00R9XZ', …(4853) ] to deeply equal []` | **0 / 12** | | `http-upload.test.ts` | — | **0 / 12** | **Mutation-tested all three assertions** — each still catches a real leak; production cleanup broken, test run, source restored: | assertion | mutation | result | |---|---|---| | `file-inputs.test.ts:249` | drop `await cleanup()` from `resolveFileInputs`' catch path | ✗ `+ [ "argent-file-input-amPJuK" ]` | | `flow-visual.test.ts:610` | drop `cleanupDiffDir(cropDir, …)` from the outer `finally` | ✗ `+ [ "argent-flow-crop-rWT9Bd" ]` | | `http-upload.test.ts:187` | drop `rm(tarPath)` from the `req.on("close")` handler | ✗ `+ [ "argent-upload-43d82021-….tar.gz" ]` | </details> <details> <summary>Sibling sweep — the same two shapes across both packages</summary> **Shape "assertion diffing a listing of `os.tmpdir()`": zero remaining.** No `readdir`/`readdirSync`/`glob` over `os.tmpdir()`, `tmpdir()` or a literal `/tmp` in either test tree. Every literal `/tmp` string left is a fixture handed to a mocked API; the three that do touch disk were checked by hand and are clean (`native-devtools-socket-bind.test.ts` mkdtemps and removes, `flow-utils.test.ts` is pure path math, `native-profiler-external-device.test.ts` writes only inside its own per-test `mkdtemp`). **Shape "a captured temp path never cleaned up": six files fixed.** Each gets one recording choke point plus an `afterEach`. Running those six under a scoped `TMPDIR` (`48 passed` both times): ``` before: 44 entries after: 1 entry ``` | file | sites | |---|---| | `screenshot-diff-tool.test.ts` | 9 | | `react-profiler/ast-index.test.ts` | 6 | | `event-log.test.ts` | 1 helper (4 dirs/run) | | `screenshot-diff.test.ts` | 1 helper | | `react-profiler/component-source-duplicates.test.ts` | 2 | | `ios-instruments/combined-report-frozen-anchor.test.ts` | 1 | | `flows/flow-visual.test.ts` | production's crop + diff dirs (5/run) | **Left deliberately (1 entry):** an **empty** `argent-chromium-media/` — production's fixed staging dir (`src/chromium-server/cdp-session.ts:218`), shared with any live chromium-server on the machine. A test `rm`ing it would be the machine-global-resource defect #714 removed elsewhere. Two further shapes turned up while checking that claim, both *production* dirs a test drove and then abandoned: | site | per run | why the test could not just `rm` it | |---|---|---| | `screenshot-diff-tool.test.ts` | 2 dirs + 2 PNGs | `resolveOutputDir` mints `argent-screenshot-diff/<hex>` before anything validates the call, so the path exists before the test can learn it — and on a throw it never learns it at all | | `flows/flow-visual.test.ts` | 5 dirs + 5 PNGs | `runSnapshot` leaves the file it registered as an artifact inside its `argent-flow-crop-*` / `argent-flow-diff-*` scratch dir; from there the dir belongs to whoever consumes the artifact, which here is the test | Both are fixed the same way, with `redirectTmpdir` (already used by seven other test files): point `os.tmpdir()` at a directory the test already owns and removes, so whatever production mints lands inside it whether the call returns or throws. The `<hex>` and `mkdtemp` suffixes are per-call and never shared, so nothing a concurrent tool-server owns is touched — verified with a decoy file planted under the shared root, which survives the run. Measured under a scoped `TMPDIR`, three consecutive runs each: `screenshot-diff-tool` 6 dirs + 6 PNGs → **0 + 0**; `flow-visual` 15 + 15 → **0 + 0**. The underlying production leak behind the first is still #790. `argent-mcp` is now clean end to end: every `mkdtemp` in the package has a matching `rm`. </details> <details> <summary>Checks</summary> Run from the worktree root, exit codes read unpiped: - `npm run format` → 0, no files rewritten - `npx tsc --noEmit -p packages/tool-server/tsconfig.test.json` → 0 - `npx tsc --noEmit -p packages/argent-mcp/tsconfig.test.json` → 0 - `npm run lint` → 0 - `npm run knip` → 0 - no `it.only` / `describe.only` in the diff Suites: `argent-mcp` **90 passed**; `tool-server` run as six shards under a scoped `TMPDIR`. `lens-tools-platform-gate.test.ts` times out at the 5 s default under full-suite load and passes in isolation (`2 passed`, 3.14 s). It is not in this diff and #714 recorded the same behaviour before it. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved test isolation by consistently cleaning up temporary files and directories after each test. - Added coverage verifying generated artifacts are stored under the configured artifacts directory. - Ensured screenshot, upload, event-log, profiling, visual-flow, and diff-test artifacts are removed reliably after execution. - Standardized temporary-directory handling and cleanup across test scenarios to reduce leftover test data and improve repeatability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-09-09 12:39:02 +02:00
Ignacy Łątka	abcbce991e	fix(profiler): keep all same-named components and surface platform variants (#368) ... ## Problem The AST component index (`buildAstIndexWithDiagnostics`) is keyed by component **name**. When two files define a component with the same name — most commonly platform variants, e.g. `List` in `List.tsx` and `List` in `List.web.tsx` — the index kept whichever file the directory walk happened to reach **first** (`if (!index.has(name))`) and threw the rest away. So a lookup for `List` could return the wrong platform variant's source, with no signal that other matches even existed. And because it depended on filesystem walk order, *which* `List` survived was effectively arbitrary. Raised in review on #287. ## Fix - **Collect every match per name** instead of keeping the first walked. - **Choose a deterministic primary**: base files are preferred over `.web` / `.ios` / `.android` / `.native` variants (those only override the base when bundling for that platform), then ties break on path → line → column. The result no longer depends on directory-walk order. - **Surface the rest** as `otherMatches` on the index entry. `react-profiler-component-source` now returns `otherMatches: [{ file, line, col }]` alongside the primary, so a caller asking for `List` can tell the locations apart instead of trusting a single arbitrary file. `react-profiler-analyze` needs no change — it enriches findings with the (now deterministic) primary location, which is strictly better than before; surfacing per-finding alternates there is out of scope. ## Tests `packages/tool-server/test/react-profiler/ast-index.test.ts` (index layer) and a new `component-source-duplicates.test.ts` (end-to-end through the tool's `execute`): - same name in `List.tsx` + `List.web.tsx` → primary is the base file, its source is returned, `List.web.tsx` is under `otherMatches`; - only platform variants (`.android` + `.ios`, no base) → deterministic primary by path; - uniquely-named component → no `otherMatches`. The two collision assertions fail on pre-fix code (no `otherMatches`, arbitrary primary) and pass with the fix. Verified locally with Node 20: `vitest run test/react-profiler/` — 80 passed; `tsc --build` clean; prettier + eslint clean.	2026-06-25 21:48:16 +02:00