refactor: run atoms e2e when e2e label added#23281
Conversation
WalkthroughAdds a new reusable workflow e2e-atoms by converting .github/workflows/e2e-atoms.yml from pull_request to workflow_call, updating permissions, renaming the job to e2e-atoms, and renaming artifacts. Integrates this workflow into CI by adding an e2e-atoms job to .github/workflows/all-checks.yml and .github/workflows/pr.yml, specifying its needs and using the reusable workflow with inherited secrets. Updates gating: includes e2e-atoms in final required needs in all-checks.yml, and in merge-reports.needs and required.needs in pr.yml. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
keithwillcode
added
core
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/22/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add platform team as reviewer" took an action on this PR • (08/22/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/22/25)1 label was added to this PR based on Keith Williams's automation. |
dosubot
bot
added
automated-tests
![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 (4)
.github/workflows/atoms-e2e.yml (1)
4-4: Converted to reusable workflow correctly; verify artifact paths relative to working directory.Switch to
workflow_calllooks good and matches how you’re invoking it from other workflows.
- Heads-up: the test step runs in
packages/platform/examples/base, but both artifact uploads don’t set that working directory and use relative paths (test-results,playwright-report). Unless those folders are created at repo root, uploads will silently find nothing.Outside-changes diff to make artifact paths explicit:
- - name: Upload Test Results + - name: Upload Test Results uses: actions/upload-artifact@v4 if: always() with: name: atoms-e2e-test-results - path: test-results + path: packages/platform/examples/base/test-results retention-days: 7 - - name: Upload Playwright Report + - name: Upload Playwright Report uses: actions/upload-artifact@v4 if: always() with: name: atoms-e2e-playwright-report - path: playwright-report + path: packages/platform/examples/base/playwright-report retention-days: 7.github/workflows/all-checks.yml (1)
77-82: New atoms-e2e job wiring looks consistent with existing patterns.
uses: ./.github/workflows/atoms-e2e.yml+secrets: inheritis correct for a reusable workflow.needs: [lint, build, build-atoms, build-api-v2]is sensible to ensure Atoms and API v2 are built before running Atoms E2E.Consider aligning the runner size and
timeout-minuteshere if you anticipate heavier Atoms tests in merge queue (you can override via inputs in the reusable workflow later if needed)..github/workflows/pr.yml (2)
198-204: PR gating for atoms-e2e is wired correctly behind the label.
needs: [changes, check-label, build, build-atoms, build-api-v2]mirrors the other E2E jobs and ensures prerequisites.if: ${{ needs.check-label.outputs.run-e2e == 'true' && needs.changes.outputs.has-files-requiring-all-checks == 'true' }}matches your “run only when ready-for-e2e” intent.If Atoms tests ever need API v1, consider adding
build-api-v1toneedsnow to avoid future flakiness.
214-214: Make sure merge-reports knows about the new artifact names.You’ve added
atoms-e2etomerge-reports.needs, good. The Atoms workflow uploads artifacts named:
atoms-e2e-test-resultsatoms-e2e-playwright-reportIf
merge-reports.ymlexpects a fixed set of artifact names (e.g., from existing E2E jobs), it may not pick these up. Please verify and extend it to include the new names or to discover artifacts dynamically.I can open a follow-up to update
merge-reports.ymlonce you confirm its current artifact selection logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/all-checks.yml(1 hunks).github/workflows/atoms-e2e.yml(1 hunks).github/workflows/pr.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
.github/workflows/all-checks.yml (1)
84-84: Adding atoms-e2e to final gate is intentional and will enforce it on merge queue.Including
atoms-e2einrequired.needsmeans the merge-group workflow will fail if Atoms E2E fails. That matches the PR goal to gate on Atoms E2E when used in this workflow.Please confirm the merge-queue capacity/time impact: Atoms E2E runs on
buildjet-4vcpu-ubuntu-2204in the reusable workflow. If your merge-queue runner labels differ, you may want to override or document it..github/workflows/pr.yml (1)
259-259: Adding atoms-e2e to “required” keeps PRs blocked until Atoms E2E passes when it should run.This mirrors the existing pattern where conditional jobs (guarded by
run-e2e) still appear inrequired.needs, and the final step fails on skipped/failed jobs. That enforces adding the label before merge.To avoid confusion for contributors, consider updating contributor docs to mention that PRs changing code will require the
ready-for-e2elabel to satisfy the required checks.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-atoms.yml (1)
36-43: Artifact path likely wrong relative to working-directory; results may not upload.
yarn test:e2eruns inpackages/platform/examples/base, sotest-resultswill be created under that directory.actions/upload-artifactresolvespathrelative to the workspace root, not the step’sworking-directory. Update the path to avoid empty/missing artifacts.- name: Upload Test Results uses: actions/upload-artifact@v4 if: always() with: - name: e2e-atoms-test-results - path: test-results + name: e2e-atoms-test-results + path: packages/platform/examples/base/test-results retention-days: 7
🧹 Nitpick comments (4)
.github/workflows/e2e-atoms.yml (4)
4-4: Good move to a reusable workflow; add a fast-fail guard for required secrets.Switching to
workflow_callis aligned with the PR goal. Because callers are usingsecrets: inherit, GitHub won’t enforce a required-secrets schema here. Add a short validation step early in the job to fail fast when any expected secret/env is missing.Apply this diff to insert a guard step after checkout (adjust position as you see fit):
- uses: actions/checkout@v4 - uses: ./.github/actions/dangerous-git-checkout + - name: Validate required env + run: | + missing=() + for var in ATOMS_E2E_OAUTH_CLIENT_ID ATOMS_E2E_OAUTH_CLIENT_SECRET ATOMS_E2E_API_URL ATOMS_E2E_ORG_ID ATOMS_E2E_OAUTH_CLIENT_ID_BOOKER_EMBED DATABASE_URL DATABASE_DIRECT_URL; do + if [ -z "${!var}" ]; then missing+=("$var"); fi + done + if [ ${#missing[@]} -ne 0 ]; then + echo "Missing required env: ${missing[*]}" >&2 + exit 1 + fi - uses: ./.github/actions/yarn-install
20-23: Consider concurrency to cancel superseded runs.Atoms E2E can be slow/expensive. Add a concurrency group so pushes/label flips on the same ref don’t run multiple copies.
e2e-atoms: + concurrency: + group: e2e-atoms-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true timeout-minutes: 15 name: Atoms E2E Tests
5-7: Scope permissions at the job level (principle of least privilege).You only need
actions: writeandcontents: readfor this job. Scoping them to the job keeps the workflow safer if additional jobs get added later.-permissions: - actions: write - contents: read + # (optional) remove top-level permissions in favor of job-scoped permissionsAnd under the
e2e-atomsjob:e2e-atoms: + permissions: + actions: write + contents: read
10-10: Possible typo: 8096 MB vs 8192 MB for Node heap.If the intent was ~8 GiB, the common value is
8192. If8096is intentional, ignore.- NODE_OPTIONS: --max-old-space-size=8096 + NODE_OPTIONS: --max-old-space-size=8192
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/all-checks.yml(1 hunks).github/workflows/e2e-atoms.yml(3 hunks).github/workflows/pr.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pr.yml
- .github/workflows/all-checks.yml
🔇 Additional comments (1)
.github/workflows/e2e-atoms.yml (1)
40-40: No stale references to oldatoms-e2eidentifiers detected
Ran the provided ripgrep checks against the entire repository and found no occurrences of the oldatoms-e2e(-test-results|-playwright-report)?or the newe2e-atoms(-test-results|-playwright-report)?identifiers outside of the updated workflow file.Please manually verify any external consumers—such as merge-reporting scripts, dashboards, or downstream workflows in other repositories—to ensure they reference the new
e2e-atoms-*names and job ID.
- Dashboards or reporting tools pulling artifact names
- CI/CD orchestration or follow-up workflows in other repos
- Documentation or README snippets that mention the old job/artifact IDs
keithwillcode
deleted the
lauris/cal-6305-chore-run-atoms-e2e-only-when-ready-for-e2e-label-added
branch
E2E results are ready! |
2 participants
No description provided.