Repository navigation
test(cli): run the real npx shim check only in its own CI job - #6969
Conversation
There was a problem hiding this comment.
Requesting changes at d4b4db9c.
Blocker: this removes the only real shim execution, though the body says that coverage remains. The body says "Real execution of the cmd shim stays covered by the CLI: npx shim (windows-latest) job". That job (.github/workflows/ci.yml:845, cli-npx-shim) runs only src/utils/npxCommand.test.ts src/commands/skills.test.ts. skills.test.ts mocks node:child_process (spawn/execFile/execFileSync are vi.fn()). So the test deleted here was the only thing in that job that ran cmd.exe /d /s /c npx.cmd for real. After this PR the job just repeats the hermetic builder assertions on three OSes, and nothing in CI would catch a shim invocation that builds the right argv but doesn't run on Windows. The deleted comment also said the test was kept unmocked on purpose ("it isn't reduced to a tautology by mocking"). The new host-platform test compares the default parameter with itself.
Suggested fix: keep the real-execution test but run it only in the dedicated job. For example, gate it with it.runIf(process.env.HF_NPX_SHIM_E2E === "1") (keep the 30 s / 60 s timeouts) and set HF_NPX_SHIM_E2E: "1" on the cli-npx-shim test step. The general CLI shards then stop spawning npx, so the engine-cli-2 ETIMEDOUT goes away, and the shim job still runs the real thing. If you'd rather drop real execution entirely, the body has to say that coverage is gone, and the cli-npx-shim job should be removed or given a real check.
fails without the fix: line. The line doesn't name a test that fails without the change. The two tests it names pass with or without this PR. Once the change above makes the shim job the place where real execution lives, please say which check would go red without it, or state plainly that this is a test-only change with no failing-test counterpart. Then a reviewer can verify the claim instead of having to trust it.
Not blocking: the per-platform builder tests are correct, and the hermetic shape is fine for the general shards.
Reviewed by Genesis V2 on behalf of @miguel-heygen.
|
Fixed in 0c74d36, both points. The real npx test is back, gated with it.runIf on HF_NPX_SHIM_E2E=1, and the cli-npx-shim test step sets it, so that job is again the one place that really runs cmd.exe /d /s /c npx.cmd; the general shards skip it. The body now says plainly this is a test-only change with no failing-test counterpart, and what it changes in CI. Verified on miga: 6 pass and 1 skipped without the variable, 7 pass with it (real npx --version), 3 runs. |
There was a problem hiding this comment.
Approving at 0c74d364. Both points from my earlier review are resolved.
- The real-npx test is still there. It now runs only when
it.runIf(process.env.HF_NPX_SHIM_E2E === "1")is true, with the same 30 s / 60 s timeouts. Thecli-npx-shimtest step is the only place that sets the variable.testis plainvitest run, so the variable reaches the worker. I checked theCLI: npx shim (ubuntu-latest)log on this head:npxCommand.test.ts (7 tests)passed and the real test was not skipped. The general shards skip it, soengine-cli-2no longer spawns npx. - The
fails without the fix:line now says plainly that this is a test-only change with no failing-test counterpart and names the effect. That fits atest(...)PR with no production change.
The CLI: npx shim (windows-latest) job was still running when I reviewed. The merge gate covers it.
Reviewed by Genesis V2 on behalf of @miguel-heygen.
Edit accuracy: accurate 271 (base branch 271), smooth 99 of thoseThe gate passes. Quarantined, measured but not gated (0) |
What
npxCommand.test.tsran a realcmd.exe /c npx.cmd --versionwith a 30 s timeout in every CLI shard. On the Windows runner that timed out (spawnSync cmd.exe ETIMEDOUT) and turned a required check red on an unrelated PR.The real-execution test stays, but runs only when
HF_NPX_SHIM_E2E=1, which the dedicatedCLI: npx shimjob (ubuntu, macos, windows) now sets on its test step. The general CLI shards no longer spawn npx and skip that one test; the per-platform builder tests, which assert the exact command and arguments, run everywhere.Why
Flaky required check on #6964, a CLI change that never touches npx: one ETIMEDOUT in
engine-cli-2on windows-latest. The shim job keeps the only real execution of the cmd shim, so a shim that builds the right argv but fails to run on Windows still turns that job red.Test
Without the variable the file runs 6 tests and skips the real-npx one; with
HF_NPX_SHIM_E2E=1all 7 pass, including the realnpx --version. With the win32 branch ofbuildNpxCommandbroken (/d /sdropped), the win32 npx and npm builder tests fail. 3 runs in a row.fails without the fix: test-only change with no failing-test counterpart. Its effect is that
engine-cli-2no longer spawns npx, andCLI: npx shim (windows-latest)still runs it for real.Size
Under 100 changed lines.