Conversation
- Changed the test runners for Windows and macOS to run on Ubuntu. - Introduced a Playwright container for consistent browser testing. - Added environment variable setup for Turbo SCM in pull request workflows. - Removed unnecessary caching steps and Playwright browser installation commands.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe test workflow is restructured to run on ubuntu-latest with a Playwright container image instead of native Windows and macOS runners. Manual setup steps for pnpm, Node.js, and Playwright browsers are removed. A dependency on a resolve-playwright-version job is introduced to populate the container image tag, and PR environment variable propagation is added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
github-actions
bot
added
the
github actions
yamcodes
added
infra
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if: github.event_name == 'schedule' || contains(join(github.event.pull_request.labels.*.name, ','), 'windows') | ||
| runs-on: windows-latest | ||
| needs: resolve-playwright-version | ||
| runs-on: ubuntu-latest | ||
| strategy: |
There was a problem hiding this comment.
The test-windows job now declares runs-on: ubuntu-latest and runs inside the Playwright Linux container (same pattern at lines 164‑167 for test-macos), so even when the windows/macos labels are applied or on the scheduled run, we only exercise Linux. That removes the Windows and macOS coverage this workflow used to provide (there are no other runs-on: windows/macos jobs in the repo), so platform-specific regressions will slip through while the jobs still appear to have validated those platforms.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
102-106: Consider extracting duplicated Turbo SCM setup into a reusable workflow or pattern.The "Set Turbo base/head for PRs" step is duplicated identically across
test-e2e,test-windows, andtest-macosjobs. This violates DRY and creates maintenance burden if the setup logic needs to change.Options:
- Extract into a reusable workflow (
.github/workflows/turbo-setup.yml)- Create a composite action for Turbo SCM setup
- Document this as a standard pattern for all jobs that need Turbo PR context
Also applies to: 142-146, 178-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
127-137: Clarify intent: job names imply OS-specific testing but now run in a container.
test-windowsandtest-macosjobs no longer run on native Windows/macOS runners; they now run onubuntu-latestwithin a Playwright container. This is a significant semantic change:
- If the goal is to test on actual Windows/macOS OS, this breaks that requirement. You'll only be testing the Ubuntu-based container behavior.
- If the goal is consistent Playwright browser testing, the naming should be updated to reflect that these are now containerized tests, not OS-specific tests (e.g.,
test-browser-windowsortest-playwright-windows).Verify that removing native OS runners doesn't hide OS-specific issues (e.g., path handling, platform-specific test logic, or browser behavior differences).
95-98: Approve: Consistent Playwright container setup across jobs.The container configuration is consistently applied to all Playwright-dependent jobs (
test-e2e,test-windows,test-macos) with matching image tags and user permissions. This is a solid approach for standardizing the test environment.However, verify that the
--user 1001container option doesn't cause unexpected permission issues:
- Does the container image already have this user configured?
- Are artifact upload permissions (lines 121-125, 156-160, 192-196) compatible with user 1001?
You can verify with a quick test workflow run to confirm artifact uploads succeed.
Also applies to: 135-137, 171-173
67-86: The Playwright version extraction is working correctly and requires no changes.The verification confirms the
awkcommand successfully extracts version1.56.1frompnpm-workspace.yamland produces a valid semantic version. The parsing is functioning as intended with the current repository structure, and the extracted version is ready for use in container image tags.
yamcodes
changed the title
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.