[issues/574] Rework integration test script ecosystem: rename, rewrite, consolidate#586
Merged
Conversation
…e, consolidate This reworks the integration test orchestration layer end-to-end. It renames `test-release-run.sh` to `run-integration-tests.sh`, rewrites its flag composition to resolve all filtering through the QA YAML (single source of truth), fixes multiple bugs where flag combinations silently dropped filters, removes Python as a dependency from two scripts by extending `resolve-qa-labels.js`, adds 78 bats tests across both orchestration scripts, and strips the now-redundant `qa-smoke-setup.sh` and settings profile system. Issue #575 (bats tests for validate-qa-coverage.sh) is folded in since the consolidation work touched that script. - Rename `test-release-run.sh` to `run-integration-tests.sh` (git mv), update all references in package.json, TESTING.md, and CI composite actions - Replace `[assisted]` string matching and `MOCHA_INVERT` with YAML-based TC ID lists from `resolve-qa-labels.js`. All filters produce positive ID sets, AND-combined via lookahead syntax - Fix flag composition bugs: `--grep`/`--label` with `--automated` no longer silently drops the assisted exclusion; `--no-assisted` with `--grep` now composes correctly; `--assisted` standalone resolves through YAML instead of being a silent no-op - Replace `--without-extensions` dead code with `--exclude-label` (repeatable), add `--exclude-assisted`, make `--label` repeatable - Separate `MODE` into `CONFIG_FILE`, `REPORT_MODE`, and `QA_SKIP`; remove cosmetic `COMMAND` variable; extract `strip_ansi()` for the duplicated sed call; fix `--grep` error message - Replace awk YAML parser and python3 `relpath()` in `validate-qa-coverage.sh` with `resolve-qa-labels.js` calls and bash parameter expansion - Replace Python+PyYAML in `generate-qa-issue.sh` with `resolve-qa-labels.js --json` output - Update CI composite actions: `--no-assisted` to `--exclude-assisted`, add `--exclude-label requires-extensions` to standard CI run - Fix `terminal-picker-006` QA YAML: `automated: true` → `automated: false` (scenario requires Cursor's hidden background terminals, passes vacuously in VS Code host) - Remove `qa-smoke-setup.sh` (329 lines): settings profiles it managed are all covered by automated tests; Cursor manual QA section inlines `pnpm package:vscode-extension:withInstall:both && cursor qa/fixtures/workspace` - Delete 4 settings profile JSON files (`clipboard-never`, `no-dirty-warning`, `custom-delimiters`, `terminal-picker-low`) and `qa/fixtures/settings/` directory - Remove PyYAML prerequisite from release testing instructions and generator script - Remove `qa:setup` and `qa:setup:vscode-extension` scripts from both package.json files - Move stub helpers (`stub_dir`, `make_stub`, `make_passive_stub`) to shared `test_helper.bash` - Add 78 bats tests: 59 for `run-integration-tests.sh` (all flag combinations, exit codes, report files, ANSI stripping, re-run commands) and 19 for `validate-qa-coverage.sh` (YAML auto-discovery, all 4 asymmetry types, report generation, counts, edge cases) - Documentation: TESTING.md updated for new flags, script name, YAML-based filtering, and simplified QA cycle diagram; DEVELOPMENT.md updated with inline QA commands; CHANGELOG: not needed (internal infrastructure changes); README: not needed (no user-facing features) - All 78 bats tests pass - Full Jest suite passes (98.44% statements, 95.49% branches) - pnpm test:release (VS Code host integration tests): exercised separately; no behavior changes to tests themselves - Closes #574 - Closes #575
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR migrates QA tooling from Python to Node, adds a unified run-integration-tests.sh runner, refactors validate-qa-coverage to use resolve-qa-labels.js, updates CI action flags and package scripts, adjusts fixture/settings usage, and adds extensive Bats tests and test helpers. ChangesQA Testing Infrastructure Modernization
Sequence DiagramsequenceDiagram
participant Dev as Developer CLI
participant Runner as run-integration-tests.sh
participant Resolver as resolve-qa-labels.js
participant VSCode as npx vscode-test
participant Validator as pnpm validate:qa-coverage
participant Report as qa/output report
Dev->>Runner: invoke runner with flags (--automated/--label/--grep/--with-extensions/--assisted/--exclude-assisted)
Runner->>Resolver: node resolve-qa-labels.js [--label/--exclude-label/--assisted/--exclude-assisted] (when filtering)
Resolver-->>Runner: matching test IDs (or JSON groups)
Runner->>VSCode: npx vscode-test $CONFIG (MOCHA_GREP env) | tee -> Report
VSCode-->>Report: ANSI-stripped test output
alt filtering not active
Runner->>Validator: pnpm validate:qa-coverage
Validator-->>Runner: validation exit code
end
Runner-->>Dev: FINAL_EXIT (max of test and validator)
alt FINAL_EXIT != 0
Runner-->>Dev: print rerun command with extracted failed IDs and same flags
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ QA Coverage OK No user-visible changes were found in the PR. Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
CI / Test & Validate — run summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js (1)
159-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize quoted
automatedvalues before comparing.This now preserves surrounding quotes, so valid YAML like
automated: "true"orautomated: 'assisted'will be skipped by--automated-only,--assisted,--exclude-assisted, and the JSON totals. That breaks the quoted-scalar cases called out in the QA coverage objectives.Suggested fix
+const normalizeScalar = (value) => value.trim().replace(/^['"]|['"]$/g, ''); + const autoMatch = line.match(/^\s+automated:\s*(.+)$/); if (autoMatch) { - currentCase.automated = autoMatch[1].trim(); + currentCase.automated = normalizeScalar(autoMatch[1]); inLabels = false; continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js` around lines 159 - 161, The code assigns currentCase.automated = autoMatch[1].trim() but preserves surrounding quotes, which breaks downstream comparisons; update the assignment to normalize the value by trimming whitespace then stripping a single matching pair of surrounding quotes (single or double) if present, e.g., get the raw = autoMatch[1].trim(), if raw starts and ends with the same quote character (' or ") remove those outer quotes, and then assign the unquoted value to currentCase.automated so comparisons like --automated-only, --assisted, --exclude-assisted, and JSON totals work as expected.packages/rangelink-vscode-extension/scripts/generate-qa-issue.sh (1)
134-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
requires_extensionsis never taking the with-extensions branch.The JSON field is a boolean, so
String(...)producestrue/false, notTrue/False. This condition never matches, and extension-dependent groups get the wrong command in the checklist.Suggested fix
- if [[ "$REQ_EXT" == "True" ]]; then + if [[ "$REQ_EXT" == "true" ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/scripts/generate-qa-issue.sh` around lines 134 - 152, The variable REQ_EXT is set from the JSON boolean requires_extensions but the script compares it to "True" (capital T), so the branch for with-extensions never runs; update the comparison or normalize REQ_EXT so it matches JSON booleans (e.g., emit or compare against "true" lowercase) and keep the existing GROUP_CHECKBOXES construction that uses the with-extensions command for REQ_EXT truthy values; specifically, adjust the node one-liner that sets REQ_EXT or change the if test that checks REQ_EXT so it correctly recognizes JSON true/false for requires_extensions and toggles between the two pnpm commands when building GROUP_CHECKBOXES.
🧹 Nitpick comments (2)
tests/shell/run-integration-tests.bats (1)
612-635: ⚡ Quick winRe-run test doesn’t verify re-run command generation yet.
This test only checks exit status, so it can pass even if rerun-command output regresses. Add an assertion for the rerun block/command text (stdout or generated report content) to cover the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shell/run-integration-tests.bats` around lines 612 - 635, The test "re-run: generated on failure with failed TC IDs" currently only asserts exit status and can miss regressions in the rerun command output; update the test (in the block named re-run: generated on failure with failed TC IDs) to assert that the rerun block/command is produced by checking either stdout from run "$SCRIPT" or the contents of "$TEST_TEMP_DIR/report-mock.txt" for the expected rerun text (the generated mocha/VSCode re-run command or the specific failed TC IDs like clipboard-preservation-001 and bind-to-destination-005); add one or more assertions after run "$SCRIPT" that verify the rerun command string appears (e.g. using grep/[[ ]] or bats' [[ "$output" =~ ... ]]) so the test fails when rerun generation regresses.tests/shell/test_helper.bash (1)
25-36: ⚡ Quick win
make_stubcontract is misleading and partially dead.Line 26–29 documents generic
exit_var/output_varbehavior, but Line 31–32 values are unused and Line 34 just writes stdin verbatim. Please either implement the documented behavior or remove those parameters/docs to avoid misuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shell/test_helper.bash` around lines 25 - 36, The make_stub function currently ignores the documented exit_var/output_var parameters; update make_stub (and the generated stub file code) so the created stub records its args to "$TEST_TEMP_DIR/<name>-args", writes the contents of the caller-specified output variable (use indirect expansion ${!output_var}) to stdout (fall back to consuming stdin if empty), and exits with the numeric value from the caller-specified exit variable (use ${!exit_var:-0}); also support an optional extra_body appended to the generated stub before chmod +x so callers can add custom behavior. Use the unique names make_stub, STUB_DIR, TEST_TEMP_DIR, exit_var, and output_var to locate where to change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rangelink-vscode-extension/scripts/generate-release-testing-instructions.sh`:
- Around line 177-180: Update the QA checklist command so it forwards arguments
defensively like Phase 2: change the invocation of the npm script
generate:qa-issue:vscode-extension to include the explicit separator (i.e., use
-- before script args) so the local flag is passed to the script; replace the
current `pnpm generate:qa-issue:vscode-extension --local` usage with `pnpm
generate:qa-issue:vscode-extension -- --local` to match the `-- --dry-run`
pattern used elsewhere.
In `@packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js`:
- Around line 21-23: The usage string in resolve-qa-labels.js does not mention
the --json flag even though the parser accepts it; update the usage/help text
(the multiline usage string near the top of scripts/resolve-qa-labels.js) to
include --json as an output option (e.g. add '[--json]' or ' [--format json]'
alongside the existing format options) so the --help output advertises the JSON
mode; ensure the added token matches the parser's expected flag name (--json).
In `@packages/rangelink-vscode-extension/scripts/run-integration-tests.sh`:
- Around line 286-296: The re-run command builder in run-integration-tests.sh
omits the CONFIG_AUTOMATED flag so rebuilt commands lose automated-mode
settings; update the RERUN_CMD assembly (the block that appends LABEL_FILTERS,
EXCLUDE_LABELS, ASSISTED_ONLY, EXCLUDE_ASSISTED, CONFIG_EXTENSIONS) to also
check CONFIG_AUTOMATED and, when true, append the --automated flag (similar to
how [[ "$ASSISTED_ONLY" == true ]] && RERUN_CMD="$RERUN_CMD --assisted") so the
generated RERUN_CMD preserves the original automated run behavior.
- Around line 37-107: The report header loses the original CLI args because the
while loop consumes "$@"; before the argument-parsing loop (the while [[ $# -gt
0 ]]; do ... done block) capture the original arguments into a variable (e.g.,
ORIGINAL_ARGS or ORIGINAL_ARGS_ARRAY) and then use that saved value in the
report header where $* is currently used so the full invocation can be
reproduced; place the capture immediately before the while loop and replace the
header reference to $* with the saved variable (use "${ORIGINAL_ARGS[@]}" if you
store an array or "$ORIGINAL_ARGS" if you store a single string).
- Around line 139-153: The QA_SKIP logic misses other filter flags; update the
HAS_FILTER computation so it becomes true not only when GREP_PATTERN or
LABEL_FILTERS have values but also when the exclude/assisted flags are present
(e.g. check EXCLUDE_LABELS array length, and boolean flags like ASSISTED and
EXCLUDE_ASSISTED). In other words, replace the current HAS_FILTER assignment
(which checks GREP_PATTERN and LABEL_FILTERS) with a combined check that
includes EXCLUDE_LABELS, ASSISTED, and EXCLUDE_ASSISTED so QA_SKIP accurately
reflects any active filter and validation (validate:qa-coverage) runs against
the same filtered set.
In `@packages/rangelink-vscode-extension/scripts/validate-qa-coverage.sh`:
- Around line 108-110: The manual-count grep (YAML_MANUAL_COUNT) currently only
matches unquoted " automated: false" and thus misses quoted forms; update the
assignment for YAML_MANUAL_COUNT to use an extended-regex grep that allows
optional leading whitespace, optional single or double quotes around false, and
flexible spacing around the colon (e.g. use grep -E with a pattern like
^\s*automated:\s*["']?false["']?\s*$) so YAML_MANUAL_COUNT correctly counts
unquoted, single-quoted, and double-quoted false values.
- Around line 103-104: The YAML ID lists coming from resolve-qa-labels.js must
be sorted before using comm: update the assignments for YAML_AUTOMATED_IDS and
YAML_ASSISTED_IDS to pipe the node output through sort (e.g.,
YAML_AUTOMATED_IDS=$(node "$SCRIPT_DIR/resolve-qa-labels.js" --yaml "$YAML_PATH"
--automated-only | sort) and similarly for YAML_ASSISTED_IDS) so both inputs to
comm are in sorted order and avoid false mismatches or "not in sorted order"
errors.
In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts`:
- Around line 72-75: The workspace search currently passes undefined for the
exclude parameter to ideInstance.workspace.findFiles (call near
AMBIGUITY_THRESHOLD) which lets user settings include node_modules and causes
false FILENAME_AMBIGUOUS results; update resolveWorkspacePath.ts to pass an
explicit NODE_MODULES_EXCLUDE_GLOB (or similar constant) as the exclude argument
to ideInstance.workspace.findFiles so node_modules matches are always ignored,
and add a test (e.g., filename-fallback-navigation-005) that places an identical
bare filename in node_modules and in src to assert the search ignores
node_modules and does not mark the name ambiguous.
In
`@packages/rangelink-vscode-extension/test-fixtures/dummy-ai-extension/extension.js`:
- Around line 68-72: There is a duplicate registration of the same VS Code
command ID "dummyAi.focusFail" via vscode.commands.registerCommand which will
cause runtime errors; remove the second registerCommand call (the block that
throws new Error('Simulated focus failure for integration testing')) so the
command is registered only once (the original registration that throws the
TC-010 clipboard-preservation test error should remain), and if you intended a
single deterministic error message make sure the remaining registration's thrown
Error text matches the TC-010 test message exactly.
In `@tests/shell/run-integration-tests.bats`:
- Around line 220-221: The negative assertions use a negated grep plus a no-op
fallback (e.g., the line '! grep -q "validate:qa-coverage"
"$TEST_TEMP_DIR/pnpm-args" || true'), which makes them always pass; remove the
trailing '|| true' from these grep lines (and the other similar occurrences that
check absence: the patterns around the other grep invocations shown in the diff)
so the negated 'grep -q' can fail the test when the forbidden string is present
(i.e., let '! grep -q ...' enforce the assertion rather than being
short-circuited).
In `@tests/shell/validate-qa-coverage.bats`:
- Line 223: The regex assertion that checks for the literal token "[assisted]"
treats the brackets as a character class; update the test assertion (the [[
"$output" =~ "…"]]) to either escape the brackets (e.g., use \[assisted\]) or
replace the regex match with a glob-style equality ([[ "$output" == *"[assisted]
test exists but not marked automated: assisted in YAML"* ]]) so the literal
"[assisted]" is matched exactly.
---
Outside diff comments:
In `@packages/rangelink-vscode-extension/scripts/generate-qa-issue.sh`:
- Around line 134-152: The variable REQ_EXT is set from the JSON boolean
requires_extensions but the script compares it to "True" (capital T), so the
branch for with-extensions never runs; update the comparison or normalize
REQ_EXT so it matches JSON booleans (e.g., emit or compare against "true"
lowercase) and keep the existing GROUP_CHECKBOXES construction that uses the
with-extensions command for REQ_EXT truthy values; specifically, adjust the node
one-liner that sets REQ_EXT or change the if test that checks REQ_EXT so it
correctly recognizes JSON true/false for requires_extensions and toggles between
the two pnpm commands when building GROUP_CHECKBOXES.
In `@packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js`:
- Around line 159-161: The code assigns currentCase.automated =
autoMatch[1].trim() but preserves surrounding quotes, which breaks downstream
comparisons; update the assignment to normalize the value by trimming whitespace
then stripping a single matching pair of surrounding quotes (single or double)
if present, e.g., get the raw = autoMatch[1].trim(), if raw starts and ends with
the same quote character (' or ") remove those outer quotes, and then assign the
unquoted value to currentCase.automated so comparisons like --automated-only,
--assisted, --exclude-assisted, and JSON totals work as expected.
---
Nitpick comments:
In `@tests/shell/run-integration-tests.bats`:
- Around line 612-635: The test "re-run: generated on failure with failed TC
IDs" currently only asserts exit status and can miss regressions in the rerun
command output; update the test (in the block named re-run: generated on failure
with failed TC IDs) to assert that the rerun block/command is produced by
checking either stdout from run "$SCRIPT" or the contents of
"$TEST_TEMP_DIR/report-mock.txt" for the expected rerun text (the generated
mocha/VSCode re-run command or the specific failed TC IDs like
clipboard-preservation-001 and bind-to-destination-005); add one or more
assertions after run "$SCRIPT" that verify the rerun command string appears
(e.g. using grep/[[ ]] or bats' [[ "$output" =~ ... ]]) so the test fails when
rerun generation regresses.
In `@tests/shell/test_helper.bash`:
- Around line 25-36: The make_stub function currently ignores the documented
exit_var/output_var parameters; update make_stub (and the generated stub file
code) so the created stub records its args to "$TEST_TEMP_DIR/<name>-args",
writes the contents of the caller-specified output variable (use indirect
expansion ${!output_var}) to stdout (fall back to consuming stdin if empty), and
exits with the numeric value from the caller-specified exit variable (use
${!exit_var:-0}); also support an optional extra_body appended to the generated
stub before chmod +x so callers can add custom behavior. Use the unique names
make_stub, STUB_DIR, TEST_TEMP_DIR, exit_var, and output_var to locate where to
change the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ca5c41e-85a4-4e67-aba1-4243c6cf34b6
📒 Files selected for processing (24)
.github/actions/run-integration-tests-with-extensions/action.yml.github/actions/run-integration-tests/action.ymlpackage.jsonpackages/rangelink-vscode-extension/DEVELOPMENT.mdpackages/rangelink-vscode-extension/TESTING.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/qa/fixtures/settings/clipboard-never.jsonpackages/rangelink-vscode-extension/qa/fixtures/settings/custom-delimiters.jsonpackages/rangelink-vscode-extension/qa/fixtures/settings/no-dirty-warning.jsonpackages/rangelink-vscode-extension/qa/fixtures/settings/terminal-picker-low.jsonpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/scripts/generate-qa-issue.shpackages/rangelink-vscode-extension/scripts/generate-release-testing-instructions.shpackages/rangelink-vscode-extension/scripts/qa-smoke-setup.shpackages/rangelink-vscode-extension/scripts/resolve-qa-labels.jspackages/rangelink-vscode-extension/scripts/run-integration-tests.shpackages/rangelink-vscode-extension/scripts/test-release-run.shpackages/rangelink-vscode-extension/scripts/validate-qa-coverage.shpackages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.tspackages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.tspackages/rangelink-vscode-extension/test-fixtures/dummy-ai-extension/extension.jstests/shell/run-integration-tests.batstests/shell/test_helper.bashtests/shell/validate-qa-coverage.bats
💤 Files with no reviewable changes (7)
- packages/rangelink-vscode-extension/qa/fixtures/settings/custom-delimiters.json
- packages/rangelink-vscode-extension/qa/fixtures/settings/no-dirty-warning.json
- packages/rangelink-vscode-extension/qa/fixtures/settings/terminal-picker-low.json
- packages/rangelink-vscode-extension/scripts/test-release-run.sh
- packages/rangelink-vscode-extension/qa/fixtures/settings/clipboard-never.json
- packages/rangelink-vscode-extension/scripts/qa-smoke-setup.sh
- package.json
Comment on lines
177
to
+180
| Generate a local QA checklist: | ||
|
|
||
| \`\`\`bash | ||
| pnpm generate:qa-issue:vscode-extension -- --local | ||
| pnpm generate:qa-issue:vscode-extension --local |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/rangelink-vscode-extension/scripts/generate-release-testing-instructions.sh | grep -A 20 -B 20 "Phase 2\|Phase 5\|dry-run\|--local"Repository: couimet/rangeLink
Length of output: 5589
🌐 Web query:
pnpm argument forwarding npm scripts -- separator behavior
💡 Result:
In pnpm, argument forwarding to npm scripts generally works by appending any arguments provided after the script name directly to the command defined in your package.json [1][2][3]. Unlike npm, which often requires the -- separator to distinguish between arguments for the package manager and arguments for the script, pnpm is designed to forward these arguments automatically [4][1][2]. Key behaviors and considerations include: Automatic Forwarding: When you run a command like pnpm run <script> --arg1, pnpm appends --arg1 to the underlying script command [1][2]. You typically do not need the -- separator [4][1]. The -- Separator: While often unnecessary, the -- separator can still be used in pnpm [4]. It is sometimes required in specific environments (such as certain PowerShell configurations) or when you need to explicitly prevent pnpm from interpreting subsequent flags as its own options [5][4][6]. Inconsistencies: Users have occasionally reported inconsistencies, particularly with shorthand commands (e.g., pnpm test vs. pnpm run test) or when using complex nested scripts [4][6]. If a command fails due to an "Unknown option" error, it is often because pnpm is attempting to parse the arguments as its own CLI flags rather than passing them to the script [5][4][2]. In such cases, inserting the -- separator before the arguments usually resolves the issue by forcing pnpm to stop parsing flags [4]. Best Practice: If you encounter issues with argument parsing, explicitly using the -- separator is the recommended way to ensure arguments are passed correctly to the underlying script [4].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://stackoverflow.com/questions/74102152/how-to-pass-arbitrary-argument-with-pnpm
- 3: Pass additional command line arguments when running scripts pnpm/pn#48
- 4: Args forwarding with
pnpm run testandpnpm testis inconsistent pnpm/pnpm#4821 - 5: Running scripts in cli: Pass the args after
--to the program called pnpm/pnpm#2937 - 6: Run scripts without
--? pnpm/pnpm#3778
Use -- consistently with Phase 2 for defensive argument forwarding.
Line 180 uses pnpm generate:qa-issue:vscode-extension --local without the -- separator, while Phase 2 (line 136) uses pnpm generate:qa-issue:vscode-extension -- --dry-run. Although pnpm generally forwards arguments automatically, using -- explicitly (as in Phase 2) is the safer approach to ensure arguments reach the script rather than being interpreted as pnpm flags. For consistency and clarity, apply the same pattern here: pnpm generate:qa-issue:vscode-extension -- --local.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rangelink-vscode-extension/scripts/generate-release-testing-instructions.sh`
around lines 177 - 180, Update the QA checklist command so it forwards arguments
defensively like Phase 2: change the invocation of the npm script
generate:qa-issue:vscode-extension to include the explicit separator (i.e., use
-- before script args) so the local flag is passed to the script; replace the
current `pnpm generate:qa-issue:vscode-extension --local` usage with `pnpm
generate:qa-issue:vscode-extension -- --local` to match the `-- --dry-run`
pattern used elsewhere.
Comment on lines
72
to
75
| const matches = await ideInstance.workspace.findFiles( | ||
| pattern, | ||
| '{**/node_modules/**}', | ||
| undefined, | ||
| AMBIGUITY_THRESHOLD, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the VS Code API, for workspace.findFiles(include, exclude, maxResults), what default excludes are applied when excludeisundefined? Does that include node_modules by default, or only settings-based excludes (and which settings)?
💡 Result:
In the VS Code API, when the exclude argument for workspace.findFiles(include, exclude, maxResults) is set to undefined, the method automatically applies default file-exclusion patterns [1][2]. These default excludes are primarily derived from the files.exclude setting [1][2]. They do not include the search.exclude setting by default [1][2]. Regarding node_modules: While node_modules is frequently included in the default files.exclude configuration in many VS Code environments (often as /node_modules), it is not hardcoded into the findFiles API itself [1][3]. Instead, findFiles respects whatever patterns are currently defined in the user's or workspace's files.exclude settings [2]. If files.exclude is empty or does not contain a pattern matching node_modules, findFiles will include those files in its results [3]. To explicitly disable these default excludes, you can pass null as the exclude argument [2]. If you wish to apply no excludes at all, null is the correct value to use [2]. If you provide a string pattern, the files.exclude settings are still applied in addition to your provided pattern [2].
Citations:
- 1: Revamp findFiles microsoft/vscode#48674
- 2: workspace.findFiles with exclude pattern behaves differently than documented microsoft/vscode#151211
- 3: Revamp findFiles microsoft/vscode#48674
🏁 Script executed:
cat -n packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts | head -100Repository: couimet/rangeLink
Length of output: 4105
🏁 Script executed:
# Get more context around lines 72-75, including the function signature and full context
sed -n '40,85p' packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.tsRepository: couimet/rangeLink
Length of output: 1729
🏁 Script executed:
# Check if there are other uses of findFiles in the codebase
rg "findFiles" packages/rangelink-vscode-extension/src/ -B 2 -A 2Repository: couimet/rangeLink
Length of output: 6261
🏁 Script executed:
cat packages/rangelink-vscode-extension/src/__integration-tests__/suite/filenameOnlyNavigation.test.tsRepository: couimet/rangeLink
Length of output: 6574
🏁 Script executed:
# Check if there are any issues or PR references mentioning Issue `#342` or node_modules problems
rg "342|node_modules" packages/rangelink-vscode-extension/ -iRepository: couimet/rangeLink
Length of output: 987
🏁 Script executed:
# Look at the package.json to understand the file structure and any files.exclude settings
cat packages/rangelink-vscode-extension/.vscode/settings.json 2>/dev/null || echo "Not found"Repository: couimet/rangeLink
Length of output: 71
🏁 Script executed:
cat packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml | grep -A 50 "Issue `#342`"Repository: couimet/rangeLink
Length of output: 2708
🏁 Script executed:
# Also check if there are any other QA YAML files that might have updated test cases
fd "qa-test-cases.*\.yaml" packages/rangelink-vscode-extension/Repository: couimet/rangeLink
Length of output: 126
Exclude node_modules explicitly for deterministic bare-filename search.
Using undefined for the exclude parameter relies on the user's files.exclude settings, which may not include node_modules. This causes the search to potentially include dependency files, producing false FILENAME_AMBIGUOUS errors for common filenames. The proposed fix is sound — add an explicit NODE_MODULES_EXCLUDE_GLOB pattern to guarantee deterministic behavior.
The integration tests and QA scenarios do not currently cover the case where a bare filename matches a file in node_modules and another in the source tree. Consider adding a test scenario (e.g., filename-fallback-navigation-005) that validates the ambiguity detection correctly ignores node_modules matches, regardless of user settings.
Proposed fix
+const NODE_MODULES_EXCLUDE_GLOB = '{**/node_modules/**}';
+
const matches = await ideInstance.workspace.findFiles(
pattern,
- undefined,
+ NODE_MODULES_EXCLUDE_GLOB,
AMBIGUITY_THRESHOLD,
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts` around
lines 72 - 75, The workspace search currently passes undefined for the exclude
parameter to ideInstance.workspace.findFiles (call near AMBIGUITY_THRESHOLD)
which lets user settings include node_modules and causes false
FILENAME_AMBIGUOUS results; update resolveWorkspacePath.ts to pass an explicit
NODE_MODULES_EXCLUDE_GLOB (or similar constant) as the exclude argument to
ideInstance.workspace.findFiles so node_modules matches are always ignored, and
add a test (e.g., filename-fallback-navigation-005) that places an identical
bare filename in node_modules and in src to assert the search ignores
node_modules and does not mark the name ambiguous.
| write_test_file "suite.test.ts" <<< "test('[assisted] foo-001: does things', () => {});" | ||
|
|
||
| run "$SCRIPT" | ||
| [[ "$output" =~ "[assisted] test exists but not marked automated: assisted in YAML" ]] |
Contributor
There was a problem hiding this comment.
Escape literal [assisted] in regex assertion.
Line 223 uses a regex where [assisted] is treated as a character class. Escape brackets (or switch to == *...*) so the test validates the literal token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/shell/validate-qa-coverage.bats` at line 223, The regex assertion that
checks for the literal token "[assisted]" treats the brackets as a character
class; update the test assertion (the [[ "$output" =~ "…"]]) to either escape
the brackets (e.g., use \[assisted\]) or replace the regex match with a
glob-style equality ([[ "$output" == *"[assisted] test exists but not marked
automated: assisted in YAML"* ]]) so the literal "[assisted]" is matched
exactly.
CI / Test & Validate — run summary
To re-run failed tests: |
CI / Integration Tests (with extensions) — run summary
To re-run failed tests: |
CI / Integration Tests (with extensions) — run summary
To re-run failed tests: |
Fixes JSON boolean matching in generate-qa-issue.sh where String(true) produces "true" not "True", captures invocation args before the arg-parsing loop consumes them in run-integration-tests.sh, expands filter detection so standalone --exclude-label/--assisted/--exclude-assisted correctly skip QA validation, adds --automated to the failure re-run command, uses POSIX grep patterns and guards empty-array for-loops against set -u crashes, sorts YAML IDs before comm input in validate-qa-coverage.sh, removes the duplicate dummyAi.focusFail registration that broke extension activation, normalizes quoted YAML scalars in resolve-qa-labels.js, fixes misleading make_stub documentation, adds -- separator for pnpm parity, removes || true from negated grep assertions that made them unconditional passes, and strengthens re-run test assertions. Benefits: - generate-qa-issue.sh extension-requiring groups now get the correct pnpm command - Re-run commands reproduce the original config (timeout, config file) - Report headers preserve invocation arguments for reproducibility - QA validation skipped consistently for all filter types - validate-qa-coverage.sh works on macOS (comm requires sorted input, BSD grep rejects \s and \+) - dummy-ai-extension activates correctly with all registered commands available - Negated grep assertions in bats tests actually enforce absence Ignored Feedback: - resolveWorkspacePath.ts node_modules exclude: VSCode default files.exclude already covers node_modules; a user who removes it made a conscious choice - validate-qa-coverage.bats bracket character class: quoted =~ right-hand side is treated as a literal string by bash, not a regex character class; the test works correctly Ref: #586 (review)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js`:
- Around line 178-181: The code assigns currentCase.scenario from
scenarioMatch[1] but only strips single quotes; update the assignment in the
block that handles the scenario match (where currentCase.scenario is set) to
also remove surrounding double quotes so quoted YAML values like "..." are
normalized; specifically, after trimming scenarioMatch[1], strip any leading or
trailing single OR double quote characters before assigning to
currentCase.scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7e9fcf3-fed5-4dc2-8a06-e2dc1b532542
📒 Files selected for processing (8)
packages/rangelink-vscode-extension/TESTING.mdpackages/rangelink-vscode-extension/scripts/generate-qa-issue.shpackages/rangelink-vscode-extension/scripts/resolve-qa-labels.jspackages/rangelink-vscode-extension/scripts/run-integration-tests.shpackages/rangelink-vscode-extension/scripts/validate-qa-coverage.shpackages/rangelink-vscode-extension/test-fixtures/dummy-ai-extension/extension.jstests/shell/run-integration-tests.batstests/shell/test_helper.bash
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/shell/run-integration-tests.bats
- packages/rangelink-vscode-extension/TESTING.md
- packages/rangelink-vscode-extension/scripts/run-integration-tests.sh
- packages/rangelink-vscode-extension/scripts/generate-qa-issue.sh
- packages/rangelink-vscode-extension/scripts/validate-qa-coverage.sh
CI / Test & Validate — run summary
To re-run failed tests: |
The automated field already strips both single and double quotes from YAML scalars, but feature and scenario only stripped single quotes. Ten scenario entries in the QA YAML use double quotes, causing surviving quote characters in downstream --json output consumed by generate-qa-issue.sh. Ref: #586 (review)
CI / Test & Validate — run summary
To re-run failed tests: |
CI / Integration Tests (with extensions) — run summary
To re-run failed tests: |
CI / Integration Tests (with extensions) — run summary
To re-run failed tests: |
Since resetRangelinkSettings already restores defaults before every test via standardSuite.setup(), loadSettingsProfile was redundant indirection — reading JSON fixture files to apply settings that tests can set directly with config.update().
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.ts (1)
518-521: ⚡ Quick winUse one constant for
maxInlinevalue to avoid drift.
2is hardcoded inconfig.updateand log, then redefined asLOW_MAX_INLINE. Define once and reuse for update/assertions/logging.♻️ Proposed cleanup
- const config = vscode.workspace.getConfiguration(); - await config.update('rangelink.terminalPicker.maxInline', 2, vscode.ConfigurationTarget.Global); - log('set rangelink.terminalPicker.maxInline to 2'); - const LOW_MAX_INLINE = 2; + const LOW_MAX_INLINE = 2; + const config = vscode.workspace.getConfiguration(); + await config.update( + 'rangelink.terminalPicker.maxInline', + LOW_MAX_INLINE, + vscode.ConfigurationTarget.Global, + ); + log(`set rangelink.terminalPicker.maxInline to ${LOW_MAX_INLINE}`);As per coding guidelines "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.ts` around lines 518 - 521, Replace the duplicated literal 2 with a single named constant and reuse it for the config update, log message, and assertions: introduce a SCREAMING_SNAKE_CASE constant (e.g., LOW_MAX_INLINE) declared once in the test scope, then call vscode.workspace.getConfiguration().update('rangelink.terminalPicker.maxInline', LOW_MAX_INLINE, vscode.ConfigurationTarget.Global), use LOW_MAX_INLINE in the log string and any subsequent assertions; update references to the old hardcoded `2` so all three places (config.update, log, and the variable) reference the same constant (symbols: vscode.workspace.getConfiguration, config.update, LOW_MAX_INLINE).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.ts`:
- Around line 518-521: Replace the duplicated literal 2 with a single named
constant and reuse it for the config update, log message, and assertions:
introduce a SCREAMING_SNAKE_CASE constant (e.g., LOW_MAX_INLINE) declared once
in the test scope, then call
vscode.workspace.getConfiguration().update('rangelink.terminalPicker.maxInline',
LOW_MAX_INLINE, vscode.ConfigurationTarget.Global), use LOW_MAX_INLINE in the
log string and any subsequent assertions; update references to the old hardcoded
`2` so all three places (config.update, log, and the variable) reference the
same constant (symbols: vscode.workspace.getConfiguration, config.update,
LOW_MAX_INLINE).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe76d8a4-e51a-4a85-98ce-17adb4bffba9
📒 Files selected for processing (7)
.github/actions/run-integration-tests-with-extensions/action.yml.github/actions/run-integration-tests/action.ymlpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/scripts/resolve-qa-labels.jspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/settingsHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.ts
💤 Files with no reviewable changes (1)
- packages/rangelink-vscode-extension/src/integration-tests/helpers/settingsHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/actions/run-integration-tests-with-extensions/action.yml
- .github/actions/run-integration-tests/action.yml
- packages/rangelink-vscode-extension/scripts/resolve-qa-labels.js
CI / Integration Tests (with extensions) — run summary
|
CI / Test & Validate — run summary
|
CI / Integration Tests (with extensions) — run summary
|
CI / Test & Validate — run summary
|
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.
Summary
This reworks the integration test orchestration layer end-to-end. It renames
test-release-run.shtorun-integration-tests.sh, rewrites its flag composition to resolve all filtering through the QA YAML (single source of truth), fixes multiple bugs where flag combinations silently dropped filters, removes Python as a dependency from two scripts by extendingresolve-qa-labels.js, adds 78 bats tests across both orchestration scripts, and strips the now-redundantqa-smoke-setup.shand settings profile system. Issue #575 (bats tests for validate-qa-coverage.sh) is folded in since the consolidation work touched that script.Changes
test-release-run.shtorun-integration-tests.sh(git mv), update all references in package.json, TESTING.md, and CI composite actions[assisted]string matching andMOCHA_INVERTwith YAML-based TC ID lists fromresolve-qa-labels.js. All filters produce positive ID sets, AND-combined via lookahead syntax--grep/--labelwith--automatedno longer silently drops the assisted exclusion;--no-assistedwith--grepnow composes correctly;--assistedstandalone resolves through YAML instead of being a silent no-op--without-extensionsdead code with--exclude-label(repeatable), add--exclude-assisted, make--labelrepeatableMODEintoCONFIG_FILE,REPORT_MODE, andQA_SKIP; remove cosmeticCOMMANDvariable; extractstrip_ansi()for the duplicated sed call; fix--greperror messagerelpath()invalidate-qa-coverage.shwithresolve-qa-labels.jscalls and bash parameter expansiongenerate-qa-issue.shwithresolve-qa-labels.js --jsonoutput--no-assistedto--exclude-assisted, add--exclude-label requires-extensionsto standard CI runterminal-picker-006QA YAML:automated: true→automated: false(scenario requires Cursor's hidden background terminals, passes vacuously in VS Code host)qa-smoke-setup.sh(329 lines): settings profiles it managed are all covered by automated tests; Cursor manual QA section inlinespnpm package:vscode-extension:withInstall:both && cursor qa/fixtures/workspaceclipboard-never,no-dirty-warning,custom-delimiters,terminal-picker-low) andqa/fixtures/settings/directoryqa:setupandqa:setup:vscode-extensionscripts from both package.json filesstub_dir,make_stub,make_passive_stub) to sharedtest_helper.bashrun-integration-tests.sh(all flag combinations, exit codes, report files, ANSI stripping, re-run commands) and 19 forvalidate-qa-coverage.sh(YAML auto-discovery, all 4 asymmetry types, report generation, counts, edge cases)Test Plan
Related
Summary by CodeRabbit
Tests
Documentation
Chores