fix(tests): make tests cross-platform for Windows, Linux, and macOS#565
fix(tests): make tests cross-platform for Windows, Linux, and macOS#565jSydorowicz21 wants to merge 5 commits intoRunMaestro:mainfrom
Conversation
Replace hardcoded Unix paths with path.join(), path.resolve(), and path.normalize() so assertions match native path separators on all platforms. Use path.isAbsolute() instead of regex for portability and path.delimiter instead of hardcoded ':' for PATH splitting. Affected test files (10): - cli/services/storage.test.ts (11 tests) - cli/services/agent-sessions.test.ts (9 tests) - main/agents/session-storage.test.ts (10 tests) - main/ipc/handlers/attachments.test.ts (13 tests) - main/ipc/handlers/system.test.ts (3 tests) - main/ipc/handlers/marketplace.test.ts (1 test) - main/stats/paths.test.ts (5 tests) - main/utils/cliDetection.test.ts (2 tests) - shared/pathUtils.test.ts (1 test) - process-listeners/wakatime-listener.test.ts (3 tests)
These tests make real SSH connection attempts to non-existent hosts, which can exceed the default 10s timeout under load on Linux.
Two instances of the old startsWith('/') || match(/^[A-Z]:\/) pattern
were missed in the initial cross-platform fix. Now all 5 instances
consistently use path.isAbsolute().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughApplies cross-platform path handling across many tests (using path.join/resolve/normalize/delimiter) and removes the webContents 'crashed' event handler from the window manager; changes are limited to tests, docs, and one production file edit (handler removal). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR replaces 57+ hardcoded Unix path separators across 10 test files with portable Key changes:
One minor inconsistency to consider: in Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test runs on any platform] --> B{Which OS?}
B -->|Windows| C[path.sep = backslash\npath.delimiter = semicolon\npath.resolve adds drive letter]
B -->|macOS / Linux| D[path.sep = slash\npath.delimiter = colon\npath.resolve keeps Unix root]
C --> E[path.join constructs\nnative Windows paths]
D --> F[path.join constructs\nnative Unix paths]
E --> G[Assertion uses same\npath.join / path.resolve\nas production code]
F --> G
G --> H{Assertion matches\nproduction output?}
H -->|Yes - portable| I[Test PASSES on all platforms]
H -->|No - hardcoded separator| J[Test FAILS on non-matching OS]
style I fill:#22c55e,color:#fff
style J fill:#ef4444,color:#fff
style G fill:#3b82f6,color:#fff
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const path = storage.getSessionPath('/project', 'session-id', fullConfig); | ||
| expect(path).toBe('~/.local/share/opencode/storage/message/session-id'); |
There was a problem hiding this comment.
Local variable path shadows the module import
Inside several test callbacks in this file, a local variable named path is declared that shadows the top-level import path from 'path':
const path = storage.getSessionPath('/project', 'session-id', fullConfig);This occurs in the following tests (all pre-existing, reformatted by this PR):
OpenCodeSessionStorage– "should verify SshRemoteConfig interface is properly accepted" (line ~1075)FactoryDroidSessionStorage– "should verify SshRemoteConfig interface is properly accepted" (line ~1446)FactoryDroidSessionStorage– "Remote sessions directory path" innerit(line ~1475)
None of these specific callbacks call path.<method>() after the re-declaration, so there is no runtime error today. But the shadowing is fragile — if a future maintainer adds a path.isAbsolute() or similar call inside one of these callbacks they'll get a TypeError: path.isAbsolute is not a function instead of the usual path utilities. Consider renaming the local variable to avoid confusion, e.g. sessionPath or resultPath.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/releases.md (1)
1-519:⚠️ Potential issue | 🟡 MinorThis file appears unrelated to the PR objectives.
The PR is focused on making tests cross-platform by replacing hardcoded Unix path separators with Node.js path utilities. This file contains only formatting changes to historical release notes and does not relate to the test fixes described in the PR summary.
Including unrelated documentation changes creates scope creep and makes the PR harder to review and understand. Consider moving these release note formatting changes to a separate PR focused on documentation maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` around lines 1 - 519, The release notes file docs/releases.md contains unrelated formatting changes that do not belong in this test-focused PR; revert or remove the modifications to docs/releases.md from this branch (undo the commit or reset the file) so the PR only contains the test/path-separator fixes, or move the docs changes into a separate documentation PR; ensure the branch/PR only touches the test-related files (the path-separator replacements) before re-submitting for review.
🧹 Nitpick comments (1)
docs/releases.md (1)
201-201: Use correct capitalization for "GitHub".The official name of the platform is "GitHub" with a capital "H". As per coding guidelines, this is flagged by static analysis.
📝 Proposed fix
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 201, The release note uses incorrect capitalization "Github"; update the sentence beginning with "🌳 Github Worktree support was added." to use the correct product name "GitHub" (i.e., change "Github" to "GitHub") so the text reads "🌳 GitHub Worktree support was added." and keep the rest of the sentence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/releases.md`:
- Around line 1-519: The release notes file docs/releases.md contains unrelated
formatting changes that do not belong in this test-focused PR; revert or remove
the modifications to docs/releases.md from this branch (undo the commit or reset
the file) so the PR only contains the test/path-separator fixes, or move the
docs changes into a separate documentation PR; ensure the branch/PR only touches
the test-related files (the path-separator replacements) before re-submitting
for review.
---
Nitpick comments:
In `@docs/releases.md`:
- Line 201: The release note uses incorrect capitalization "Github"; update the
sentence beginning with "🌳 Github Worktree support was added." to use the
correct product name "GitHub" (i.e., change "Github" to "GitHub") so the text
reads "🌳 GitHub Worktree support was added." and keep the rest of the sentence
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f39053ea-ee79-4808-9273-23c253b3d6e6
📒 Files selected for processing (2)
AGENTS.mddocs/releases.md
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
The crashed event was removed from Electron's WebContents API. The render-process-gone handler at line 228 already covers this case with better crash details and auto-reload logic.
|
All checks pass, good to go |
1 participant
Summary
pathmodule functions (path.join(),path.resolve(),path.normalize(),path.isAbsolute(),path.delimiter) so tests pass on Windows, Linux, and macOSosmock forsession-storage.test.ts(handledefaultexport ambiguity, add missingtmpdir)Files changed (10 test files, 0 production files)
cli/services/storage.test.ts(11 tests)cli/services/agent-sessions.test.ts(9 tests)main/agents/session-storage.test.ts(10 tests + 4 timeout fixes)main/ipc/handlers/attachments.test.ts(13 tests)main/ipc/handlers/system.test.ts(3 tests)main/ipc/handlers/marketplace.test.ts(1 test)main/stats/paths.test.ts(5 tests)main/utils/cliDetection.test.ts(2 tests)shared/pathUtils.test.ts(1 test)process-listeners/wakatime-listener.test.ts(3 tests)Test plan
Summary by CodeRabbit