mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
da61d4f2eb
Post-merge review of #707. Three findings, all from that change's own surface; no behaviour outside the test suite moves. ## 1. Nothing pins the guard `test/setup/clear-argent-env.ts` strips the developer's `ARGENT_*` overrides so unit tests assert argent's shipped defaults. On a machine with nothing exported its loop body never runs, and CI is exactly such a machine, so no suite failure can report the guard being weakened. Measured at `9dc7c7cfc`, each mutation applied alone to the shipped file: | mutation | full tool-server suite | |---|---| | body replaced with `export {}` | 350 files / 4300 passed, **exit 0** | | `startsWith("ARGENT_")` -> `startsWith("ARGENT")` | 350 files / 4300 passed, **exit 0** | | `delete process.env[name]` -> `process.env[name] = ""` | `bind-failure-telemetry.test.ts` 3 passed, **exit 0** | | entry removed from `setupFiles` | suite green | All four survive every gate: `vitest`, `eslint`, `prettier`, `tsc`, `typecheck:tests`, `knip`. `test/clear-argent-env.test.ts` re-imports the module against sentinels planted at call time, so the loop runs on demand rather than only when the developer happens to have an override exported. Each mutation now kills exactly one test, and the mapping is distinct: ``` body removed -> removes an ARGENT_ key outright instead of blanking it prefix underscore -> keeps a name that shares the prefix without the underscore delete -> blank -> removes an ARGENT_ key outright instead of blanking it setupFiles entry -> is registered as a setup file, so it runs before any test module ``` `ARGENTINA_REGION` is the look-alike: it pins that the underscore is load-bearing. The `in` check rather than a truthiness check is deliberate -- `= ""` reads as cleared to `if (process.env.X)` while still reaching a spawned child as a set key. ## 2. The header named the wrong file, and argued from the smallest cases > `ARGENT_PORT` / `ARGENT_HOST` retarget the bind that the startup-telemetry tests inspect. `startup-telemetry.test.ts` exists in that directory, mocks `createHttpApp`, and asserts nothing about host or port. `bind-failure-telemetry.test.ts` is the file that inspects the bind, and it is the one #707's own table named. A reader following the comment opens the sibling that literally bears that name and finds no bind assertion. The example list also covered only the four variables worth 9 failures, while the widest one was absent. Guard removed, one variable, full suite: ``` ARGENT_AUTH_TOKEN=devtoken -> Test Files 10 failed | 340 passed (350) Tests 80 failed | 4220 passed | 1 skipped (4301) ``` It puts every HTTP route behind a bearer check (`src/http.ts:59`), so ten files fail on status code alone. That is the strongest argument for scrubbing the whole prefix instead of a hand-maintained list, and the paragraph defending exactly that choice did not have it. ## 3. A comment #707 silently falsified `malloc-stack-logging.test.ts:89-95` promised to "restore the ambient value afterwards", capturing `process.env.ARGENT_IOS_CAPTURE` at module scope. The guard clears that variable before the module is imported, so the capture is always `undefined` and the `else` arm of the `afterEach` restore is unreachable. Clearing in `afterEach` is what the file actually needs and all it now claims. `export {}` is appended to the setup file so `tsconfig.test.json` -- which includes `test/**/*` -- treats it as a module rather than a global script; without it `typecheck:tests` fails with TS2306. ## Verification Run at `d0cb84de5`, rebased on `9dc7c7cfc`. - Full `packages/tool-server`: **351 files, 4306 passed, 1 skipped, 0 failed** on a clean environment, and identical with `ARGENT_EMULATOR_GPU_MODE=host ARGENT_EMULATOR_NO_WINDOW=1 ARGENT_SIMULATOR_NO_WINDOW=1 ARGENT_PORT=9999 ARGENT_HOST=0.0.0.0 ARGENT_AUTH_TOKEN=devtoken ARGENT_IOS_CAPTURE=all-processes` exported. - Mutation matrix above re-run against the new test: 4/4 killed, intended test each time, control green. - `prettier --check .` 0, `eslint . --max-warnings 0` 0, `tsc --build` 0, `typecheck:tests` 0, `knip` 0 (206 issues against the 215 ceiling, unchanged -- the new file is not flagged). ## Checked and left alone - The four `it.each(PLATFORMS)` retitles from #707 render correctly (`on linux ... -gpu swiftshader`); all four sites were fixed and a sweep of all 77 `.each(` sites in the repo found no other transposition. - The guard is genuinely unnecessary in the other ten packages: the union of all 53 `ARGENT_*` names read anywhere in `packages/*/src`, exported in two opposing polarities, leaves every other suite green. `argent-tools-client` already has its own copy from #714. - No CI workflow exports an `ARGENT_*` variable into a vitest process, so nothing in CI is destroyed by the scrub. `wayland-e2e.yml:46` sets `ARGENT_EMULATOR_NO_WINDOW` at job level but runs no vitest. - The other vestigial ambient save/restore pairs (`window-shake`, `http-auth`, `http-upload`, `http-flow-path-boundary`, `update-argent-tool`, `boot-device`, `boot-device-hotboot`) carry no false prose and are harmless defence-in-depth, so they stay.