Skip to content

UN-4469 [FIX] Skip optional Node groups whose node_modules is missing - #2356

Closed
athul-rs wants to merge 1 commit into
mainfrom
fix/rig-skip-optional-node-groups
Closed

athul-rs wants to merge 1 commit into
mainfrom
fix/rig-skip-optional-node-groups

Conversation

@athul-rs

Copy link
Copy Markdown
Contributor

What

  • Optional vitest / playwright rig groups skip (exit 5, "nothing collected") when their workdir has no node_modules.
  • Required Node groups are unchanged: they still run npx --no-install and fail when their install step is missing.

Why

How

  • _node_command in tests/rig/cli.py returns the existing exit-5 skip for an optional group whose workdir/node_modules is not a directory. This is the same convention the rig already uses when npx is missing.
  • The ui suite stays in the repo for local runs: install tests/e2e/ui and it runs as before.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

Database Migrations

  • None

Env Config

  • None

Relevant Docs

  • tests/groups.yaml comment on the ui group

Related Issues or PRs

Dependencies Versions

  • None

Notes on Testing

  • Three new tests in tests/rig/tests/test_cli.py: optional group without node_modules skips; required group without node_modules still runs; optional group with node_modules runs.
  • The skip test failed before the fix. With the optional guard deleted, the required-group test and both existing command-shape tests fail.
  • Rig self-tests: 118 passed, 1 skipped.
  • This PR's ci-test run is the end-to-end check: the ui group should show as skipped, not as an error.

Screenshots

  • N/A

Checklist

I have read and understood the Contribution Guidelines.

🤖 Generated with Claude Code

The `ui` Playwright group has reported 1 error and 0 tests on every CI
run since it was added in #2212: CI has npx but never installs
tests/e2e/ui, so `npx --no-install playwright test` aborts. groups.yaml
documents that a missing install must skip this optional group, but the
rig only skipped when npx itself was absent.

Optional vitest/playwright groups now skip (exit 5) when their workdir
has no node_modules. Required groups such as `frontend` still run and
fail, so a dropped install step stays red.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@sonarqubecloud

Copy link
Copy Markdown

@greptile-apps

greptile-apps Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

via Greptile

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Optional Node test groups now skip when their working directory has no node_modules.

  • Optional Node test groups skip when their packages are missing.

Reviews (1) · Last reviewed commit: "UN-4469 [FIX] Skip optional Node groups ..." · Reviewed by Greptile

@github-actions

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
✅ e2e-api-deployment e2e 3 0 0 0 8.7
✅ e2e-coowners e2e 1 0 0 0 1.3
✅ e2e-etl e2e 1 0 0 0 16.4
✅ e2e-login e2e 2 0 0 0 1.1
✅ e2e-prompt-studio e2e 1 0 0 0 4.6
✅ e2e-smoke e2e 2 0 0 0 1.0
✅ e2e-workflow e2e 1 0 0 0 14.5
✅ frontend unit 625 0 0 0 17.9
✅ integration-backend integration 638 0 0 26 47.0
✅ integration-connectors integration 1 0 0 7 7.3
✅ integration-workers integration 179 0 0 1 32.1
⚪ ui e2e 1 0 0 0 0.0
✅ unit-backend unit 1567 0 0 1 51.7
✅ unit-connectors unit 82 0 0 0 11.9
✅ unit-core unit 281 0 0 0 3.3
✅ unit-platform-service unit 18 0 0 0 2.8
✅ unit-rig unit 123 0 0 0 5.6
✅ unit-runner unit 12 0 0 0 3.2
✅ unit-sdk1 unit 843 0 0 0 43.3
✅ unit-tool-sidecar unit 8 0 0 0 1.9
✅ unit-workers unit 1445 0 0 1 125.4
TOTAL 5834 0 0 36 401.2

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • mcp-server-auth — covered by integration-backend
  • mcp-platform-auth — covered by integration-backend
  • platform-key-whoami — covered by integration-backend
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

@athul-rs

Copy link
Copy Markdown
Contributor Author

Parking this. Reviewers are split on skipping the v0 ui group: a skip would hide that the suite isn't running, while the current error at least shows it. The group doesn't block CI, and the v1 E2E group from #2355 installs Playwright itself and replaces this suite. Reopen if Hari or Jaseem want the skip before v0 is retired.

@athul-rs athul-rs closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant