Repository navigation
test(core): skip environment-dependent tests with a reason instead of failing (#28830) - #28832
Chirag6722 wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
📊 PR Size: size/L
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses test failures occurring on Windows environments by implementing capability-based guards instead of broad platform checks. By probing the host environment for necessary features like symlink support and specific shell availability, the test suite now gracefully skips tests that cannot run in a given environment, ensuring that failures are reserved for actual regressions rather than environmental limitations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces host capability probes (canCreateSymlinks and hasPowerShell7) in a new environment-capabilities.ts utility file. These probes are used to conditionally skip tests that rely on symbolic links or PowerShell 7, preventing environment-related test failures on hosts (such as default Windows installations) that lack these capabilities. Feedback on the changes suggests sanitizing PATH entries in hasPowerShell7 by removing potential surrounding double quotes to ensure robust path resolution on Windows.
| return (process.env['PATH'] ?? '').split(path.delimiter).some((entry) => { | ||
| if (entry === '') { | ||
| return false; | ||
| } | ||
| try { | ||
| return fs.existsSync(path.join(entry, 'pwsh.exe')); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
On Windows, entries in the PATH environment variable can sometimes be wrapped in double quotes (e.g., if they contain spaces). When path.join is called with a quoted entry, it produces an invalid path (e.g., "C:\\Program Files\\PowerShell\\7"\\pwsh.exe), causing fs.existsSync to return false even if PowerShell 7 is installed. Sanitizing the path entry by removing surrounding quotes ensures robust capability detection.
return (process.env['PATH'] ?? '').split(path.delimiter).some((entry) => {
const cleanEntry = entry.replace(/^"|"$/g, '');
if (cleanEntry === '') {
return false;
}
try {
return fs.existsSync(path.join(cleanEntry, 'pwsh.exe'));
} catch {
return false;
}
});… failing On a clean Windows checkout `npx vitest run` in packages/core reported 13 failures before any change. None indicated a product defect: 8 build a symlink fixture, which needs Developer Mode or an elevated shell on Windows and otherwise throws EPERM, and 4 assert the pwsh quoting behaviour, which a default Windows install cannot produce because it has only Windows PowerShell 5.1. A contributor could not tell those from a regression they had caused, and the Windows CI job is gated on github.repository so it does not run on forks, leaving no reference result to compare against. Both are now capability probes rather than platform checks. Skipping every win32 host would also skip the contributors who have Developer Mode or pwsh installed, and they are the ones most likely to be changing this code. canCreateSymlinks() creates one real link in a temp directory and caches the answer; hasPowerShell7() looks for pwsh.exe on PATH. The quoting suite's header already named its precondition in prose, so this turns that sentence into the guard. The probes live in packages/core/src/test-utils rather than the shared test-utils package: importing that barrel from a core test pulls @google/gemini-cli-core through it and fails to resolve.
ddd525f to
ccc3b18
Compare
gemini-cli
Bot
added
the
area/platform
…erShell7 Windows stores PATH entries containing spaces quoted in some setups. path.join then keeps the quote inside the path, so fs.existsSync misses a pwsh that is actually installed. The direction of that failure is the one this helper exists to avoid: a false negative skips the quoting suite on exactly the hosts that could have run it, which is the same coverage loss as guarding on platform instead of capability. Verified the mechanism: joining a quoted entry yields a path containing a literal quote and existsSync returns false, while the unquoted form resolves identically to the plain path. My own PATH carries no quoted entries, so this is defensive rather than reproduced end to end here. Raised by gemini-code-assist on google-gemini#28832.
|
Good catch, and it is the failure direction this helper exists to avoid — fixed in ddd6829. A quoted PATH entry makes Verified the mechanism rather than taking it on faith: Being straight about the limit of that check: my own PATH has 0 quoted entries, so I confirmed the mechanism and the equivalence, not an end-to-end repro on a machine that actually stores them quoted. The fix is cheap and strictly widens detection, so I took it on that basis. Guarded suites still behave: |
Shivang9983
left a comment
Shivang9983
left a comment
There was a problem hiding this comment.
LGTM on the overall approach!
Using runtime capability probes (canCreateSymlinks(), hasPowerShell7()) is significantly better than a blunt skipIf(process.platform === 'win32').
A blanket OS check unnecessarily drops Windows test coverage for contributors who have Developer Mode or modern PowerShell installed—who are precisely the folks writing and debugging Windows-specific features. Capability checks ensure tests run wherever supported and provide clear, actionable skip reasons when preconditions are missing.
|
Thanks for actually running it under PowerShell and CMD — that is the measurement that settles it, and it is worth more than the hypothesis was. Your result narrows #28830 usefully: The On the One thing on this PR for anyone picking it up: the review bot's return (process.env['PATH'] ?? '').split(path.delimiter).some((entry) => {
// Windows PATH entries containing spaces are sometimes stored quoted.
const unquoted = entry.replace(/^"|"$/g, '');Without that, a quoted entry makes |
|
Thanks @Chirag6722! Glad the PowerShell/CMD test runs helped settle the MSYS runtime collision hypothesis and reinforced the need for capability probes in #28832. I have already submitted the |
gemini-cli
Bot
added
the
priority/p2
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
Understood on the policy — could a maintainer consider adding Some context for whoever picks it up. #28830 is already labelled The failure is also confirmed by someone other than me. @Shivang9983 ran the suite on his own Windows 11 machine under native PowerShell and CMD, where the affected tests pass, which established that That distinction is the reason this PR is worth reviewing rather than closing. The alternative fix — If the policy is firm and the label is not going to be applied, I would rather know than have it time out — happy to close it myself and leave the diagnosis on #28830 for whoever picks the issue up later. |
|
Hi @maintainers, Requesting to please consider adding the I independently tested this suite on a clean Windows 11 setup across native PowerShell and CMD. The findings confirmed that:
Given that #28830 is already triaged ( |
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
2 participants
Closes #28830.
On a clean Windows checkout,
npx vitest runinpackages/corereports 13 failures before any change is made. None of them indicate a product defect — 8 need a privilege Windows does not grant by default, 4 need PowerShell 7. This makes them skip with a reason instead.The problem was never that they fail. It is that a contributor cannot tell them from a regression they just caused, and
test_windowsis gated ongithub.repository == 'google-gemini/gemini-cli', so it does not run on forks — there is no reference result to diff against.Capability probes, not platform checks
Both guards ask the host what it can do rather than what it is:
skipIf(process.platform === 'win32')would have been one line shorter and worse: it would also skip Windows contributors who do have Developer Mode orpwshinstalled, and those are exactly the people most likely to be changing this code.canCreateSymlinks()creates one real link in a temp dir and caches the result; its cleanup is wrapped so the probe can never fail a suite it exists only to describe.For the quoting suite, the precondition was already written down — in prose, in the file's own header:
This turns that sentence into the guard.
Verification
Targeted run over the six affected files, before and after:
Full
packages/coresuite before the change:14 failed | 392 passedfiles,13 failed | 7396 passedtests. After, the count drops to the one item below.prettierandeslint --max-warnings 0are clean on all changed files (the repo's pre-commit hook re-ran both on the staged set).One failure I did not fix, and am not claiming
SandboxManager Integration > automatically allows write access to .git when running git commandstill fails here:The sandbox binary shells out to msys
sh, which cannot create its shared object — almost certainly because I invoked the suite from Git Bash, so a second msys runtime was already holding it. That is an environment interaction, not a capability a probe can honestly test, and I would rather leave it visible than paper over it with a guard that hides a real failure later. Worth a separate look by someone running from PowerShell or cmd.I also would not trust a repeat full-suite run I did while another suite was still running — it produced per-test durations of 664,000ms and above, which is machine contention rather than signal. The numbers quoted above come from clean runs.
Placement note
The probes live in
packages/core/src/test-utils/rather than the sharedpackages/test-utils. I tried the shared package first; importing that barrel from acoretest fails withFailed to resolve entry for package "@google/gemini-cli-core", because the barrel re-exports modules that import core. If you would rather they were shared, that resolution issue needs solving first and is bigger than this change.Third Windows finding, not in this PR
While setting this up:
git cloneof this repo on Windows withoutcore.longpaths=trueleaves the working tree broken. Several snapshot filenames exceed the 260-character limit:git then records them as staged deletions — 3037 dirty entries in a repo that looks cloned.
git clone -c core.longpaths=truegives 0 dirty files, which confirms the cause. Nothing in CONTRIBUTING mentions it, and it is a worse first-contact failure than the test noise this PR addresses. Happy to send a docs PR for that separately if useful.