mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
82db9e2f9f
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 -->