UN-4074 [FIX] Stub npx presence in the two rig node-command tests - #2271
Open
muhammad-ali-e wants to merge 1 commit into
Open
UN-4074 [FIX] Stub npx presence in the two rig node-command tests#2271muhammad-ali-e wants to merge 1 commit into
muhammad-ali-e wants to merge 1 commit into
Conversation
`_node_command` deliberately short-circuits when Node is absent, returning the
rig's `["sh", "-c", "exit 5"]` "nothing collected" sentinel so a machine without
Node does not turn a whole run red. Both of these tests assert the real npx
invocation but never required npx to be there, so on such a machine they compare
against the sentinel and fail:
AssertionError: assert ['sh', '-c', 'exit 5'] == ['npx', ... 'vitest', 'run']
OSS CI is green because GitHub-hosted runners ship Node. It surfaces downstream:
the cloud repo's test job runs on a self-hosted runner with no Node, over a tree
merged against OSS main, so every cloud PR goes red on these two.
Stub `shutil.which` PRESENT rather than skipping. A skipif would stop them
running on exactly the machine that reported the problem; these assert command
*construction*, which should not depend on the host having Node at all. This
mirrors `test_node_command_without_npx_collects_nothing`, which stubs the same
hook to absent for the other branch — so both branches are now covered
deterministically and neither depends on the environment.
Verified with npx removed from PATH: 2 failed before, 32 passed after, and the
whole file passes both with and without npx.
|
Contributor
|
| Filename | Overview |
|---|---|
| tests/rig/tests/test_cli.py | Adds correctly targeted executable-discovery stubs so Vitest and Playwright command assertions consistently exercise their intended branches. |
Reviews (1): Last reviewed commit: "UN-4074 [FIX] Stub npx presence in the t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stub
shutil.whichpresent in the two rig node-command tests, so they assertcommand construction rather than whether the host happens to have Node installed.
Why
_node_commanddeliberately degrades when Node is absent:Both tests assert the real
npxinvocation, but neither requirednpxto bepresent. On a machine without Node they compare against the sentinel and fail:
CI here is green because GitHub-hosted runners ship Node, so this is invisible
upstream. It surfaces downstream: the enterprise repo's test job runs on a
self-hosted runner with no Node, against a tree merged with this repo's
main,so every PR there currently goes red on these two tests. Because that merge pins
main, it cannot be fixed from any branch in either repo — it has to land here.How
Stub present, rather than
skipif.A
skipifwould stop these running on exactly the machine that reported theproblem. These assert argument shape — reporter flags, argv order, that
Playwright takes its JUnit path from env rather than a flag — none of which
depends on Node being installed.
This mirrors the existing
test_node_command_without_npx_collects_nothing, whichstubs the same hook to absent to cover the other branch. Both branches are now
covered deterministically, and neither depends on the environment.
Can this PR break any existing features?
No. Test-only, scoped to two functions in one file, and it adds determinism
rather than removing coverage — the Node-less branch keeps its own dedicated
test.
Database Migrations
None.
Env Config
None.
Notes on Testing
Reproduced the downstream failure locally by running with
npxremoved fromPATH:test_cli.py, npx absenttest_cli.py, npx presentThe mutation check (reverting the stub and re-running with npx absent) reproduces
the original failure, so the change is load-bearing rather than incidental.
Related Issues or PRs
UN-4074. Found while investigating two red checks on an unrelated enterprise
chart PR; the tests themselves came in with the frontend Ant Design removal work.
Checklist