chore: Correct broken --exclude brace expansion in test scripts - #23204
Open
mydea wants to merge 1 commit into
Open
chore: Correct broken --exclude brace expansion in test scripts#23204mydea wants to merge 1 commit into
mydea wants to merge 1 commit into
Conversation
The `test`, `test:unit`, and `test:pr` scripts passed the excluded
packages as a double-quoted brace list. Shell brace expansion does not
happen inside double quotes, so Nx received the string literally and
split it on commas. The first (`@sentry-internal/{browser-integration-tests`)
and last (`bundler-plugin-integration-tests}`) entries retained the
literal braces, matched no project, and were therefore not excluded --
so `browser-integration-tests` and `bundler-plugin-integration-tests`
ran anyway.
Replace the brace shorthand with fully-qualified, comma-separated
project names, which Nx parses natively without relying on the shell.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a4e91f3. Configure here.
| "test:unit": "nx run-many -t test:unit --exclude @sentry-internal/browser-integration-tests,@sentry-internal/bun-integration-tests,@sentry-internal/deno-integration-tests,@sentry-internal/e2e-tests,@sentry-internal/integration-shims,@sentry-internal/node-integration-tests,@sentry-internal/cloudflare-integration-tests,@sentry-internal/bundler-plugin-integration-tests", | ||
| "test:update-snapshots": "nx run-many -t test:update-snapshots", | ||
| "test:pr": "nx affected -t test --exclude \"@sentry-internal/{browser-integration-tests,bun-integration-tests,deno-integration-tests,e2e-tests,integration-shims,node-integration-tests,cloudflare-integration-tests,bundler-plugin-integration-tests}\"", | ||
| "test:pr": "nx affected -t test --exclude @sentry-internal/browser-integration-tests,@sentry-internal/bun-integration-tests,@sentry-internal/deno-integration-tests,@sentry-internal/e2e-tests,@sentry-internal/integration-shims,@sentry-internal/node-integration-tests,@sentry-internal/cloudflare-integration-tests,@sentry-internal/bundler-plugin-integration-tests", |
There was a problem hiding this comment.
Missing regression test for fix
Low Severity
Flagging this because it was mentioned in the review rules file: for a fix PR, Testing Conventions expect at least one unit, integration, or E2E test that fails without the fix and passes with it. This change only updates the test / test:unit / test:pr exclude lists in package.json, with no covering regression test. A small script-level assertion (for example under test:scripts) that the exclude values stay fully qualified and free of brace expansion would lock this in.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit a4e91f3. Configure here.
Contributor
size-limit report 📦
|
mydea
marked this pull request as ready for review
August 10, 2026 08:34
Lms24
approved these changes
Aug 10, 2026
s1gr1d
approved these changes
Aug 10, 2026
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
3 participants
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.
TLDR: When running
yarn test, it would also run browser-integration-tests, even though they should be excluded. After investigating it, it turns out the exclusion was not working as it should.The
test,test:unit, andtest:prscripts intended to exclude the standalone integration-test dev-packages via a brace-list:--exclude "@sentry-internal/{browser-integration-tests,...,bundler-plugin-integration-tests}". This never worked as intended.Root cause: shell brace expansion does not occur inside double quotes, so Nx received the string verbatim and split it on commas. Every middle entry happened to match a project by its bare name, but the first entry retained a literal
{(@sentry-internal/{browser-integration-tests) and the last a literal}(bundler-plugin-integration-tests}). Those two matched no project name, so they were silently not excluded — andbrowser-integration-tests+bundler-plugin-integration-testswere scheduled and run as part ofyarn test.The fix replaces the brace shorthand with fully-qualified, comma-separated project names. Nx parses a comma-separated
--excludelist natively, so correctness no longer depends on the shell. Chose this over quoting/escaping tricks because it is unambiguous across shells and matches how Nx documents the flag.Verified: after the change,
yarn test's task graph contains 0 of the eight excluded packages (previously 2 leaked), dropping from 42 to 41 scheduled test tasks.