Repository navigation
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The change looks right to me, but I couldn't run it: it only does something on Windows, and our review sandbox is Linux. From reading the code: ShellSelect checks process.env.SHELL before anything else, so an inherited SHELL=/usr/bin/bash from Git Bash would make the shell tests pick bash while they expect PowerShell. Removing it in the test preload runs before any shell is chosen, and test/shell.test.ts sets or removes SHELL in each test itself, so its Windows and Git Bash tests aren't affected. No blocking concerns. A maintainer on Windows can confirm it by running SHELL=/usr/bin/bash bun run test test/tool-shell.test.ts in packages/core from Git Bash.
c2d1776 to
8d4f469
Compare
Issue for this PR
Fixes #54104
Type of change
What does this PR do?
Windows shell tests hard-code PowerShell syntax, but shell selection prefers
process.env.SHELL, which Git Bash sets to/usr/bin/bash. Running the core suite from Git Bash therefore selected bash and every platform-branched command failed with bash syntax errors, while CI stayed green because its bash steps don't propagateSHELL. This deletesSHELLintest/preload.tson win32 so tests exercise the platform default shell (PowerShell), the same one Windows CI resolves.Discovered while running the full core suite on Windows from Git Bash; full reproduction and CI-side evidence are in #54104.
How did you verify your code works?
On Windows 11, from a Git Bash environment with
SHELL=/usr/bin/bash, using the project runner (bun run testinpackages/core):test/tool-shell.test.tsexits 1 with/usr/bin/bash: -c: line 1: syntax error near unexpected tokenfailurestest/tool-shell.test.ts, andtest/shell.test.ts(which drivesSHELLexplicitly per test) still passesSHELLalonebun run typecheckinpackages/coreis cleanScreenshots / recordings
N/A
Checklist