test(sea): fix permission mock failure on windows runners#24973
test(sea): fix permission mock failure on windows runners#24973dogukanozen wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
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 a test failure occurring on Windows CI environments. By forcing the platform to 'linux' during a specific test case, the code ensures that the runtime permission validation logic is correctly exercised regardless of the host operating system, preventing false negatives. 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 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 counter productive. 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
|
There was a problem hiding this comment.
Code Review
This pull request modifies the sea-launch.test.js file to mock the process.platform property during a test case. The review feedback correctly identifies a risk of test leakage where the global process state might not be restored if an assertion fails. It is recommended to wrap the test logic in a try...finally block to ensure the original platform value is always restored, maintaining test isolation and reliability.
96472e0 to
a485768
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the sea-launch.test.js file to wrap a permission-related test case in a try...finally block, ensuring that the process.platform global is correctly restored after being mocked as 'linux'. Feedback focuses on improving the robustness of this mock by capturing and restoring the full property descriptor of process.platform rather than just its value, and suggests using path.posix to ensure consistent path behavior across different operating systems during the test.
a485768 to
c90952e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the sea-launch.test.js file to explicitly mock process.platform as 'linux' within the 'recreates runtime if existing has wrong permissions' test case. The implementation uses Object.defineProperty and a try...finally block to ensure the environment is correctly restored after the test. Additionally, the test dependencies now utilize path.posix for consistent path handling. I have no feedback to provide.
Summary
This PR fixes a bug where the
sea-launch.test.jsfile fails on Windows CI runners due to missing POSIX-environment mocks, ensuring robust and platform-agnostic test assertions.Details
The
prepareRuntimetest"recreates runtime if existing has wrong permissions"evaluates POSIX mode permissions (0o700vs0o777). However, theisSecurefunction insea-launch.cjsexplicitly bypasses this mode check ifprocess.platform === 'win32'. Because the test failed to mock the platform aslinux, native Windows runners evaluated the platform aswin32, skipped the POSIX check, and returnedtrue, causing the expected file deletion (rmSync) to never occur and the test to fail.This fix injects an
Object.defineProperty(process, 'platform', { value: 'linux' })wrapper into the isolated test, ensuring the assertion securely evaluates the permissions check logic regardless of the runner's native host OS.Related Issues
Closes #24965
How to Validate
npm run test:sea-launchorvitest run sea/sea-launch.test.js.recreates runtime if existing has wrong permissions.Pre-Merge Checklist