Skip to content

ci(pr): run E2E when a PR touches tests/e2e paths - #10600

Closed
innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/pr-ci-run-e2e
Closed

innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/pr-ci-run-e2e

Conversation

@innocarpe

@innocarpe innocarpe commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • Open a PR that only touches src/ → no e2e job
  • Open a PR that touches tests/e2e/** → e2e workflow runs

Closes #10518

ELI5

PRs that only touched E2E tests never ran the E2E workflow, so broken tests could look green. Path filters now trigger E2E when test/e2e paths change while leaving ordinary PRs light.

Regression specs under tests/e2e never ran on PR CI — only schedule and
release called e2e.yml — so a red regression test could merge green.
Path-filter and workflow_call the E2E suite when E2E-relevant files change.

Closes stablyai#10518
@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request workflow now includes an e2e-paths job that checks changed paths for E2E-related files and emits a should_run output. A dependent e2e job invokes the existing reusable E2E workflow when the pull request is not a draft and the output is true.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes Summary and Test plan, but it omits required Screenshots, Testing, AI Review Report, Security Audit, and Notes sections. Add the missing template sections and fill in testing, AI review, security audit, screenshots/no visual change, and notes details.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The workflow change matches #10518 by running E2E on PRs that touch the targeted E2E paths.
Out of Scope Changes check ✅ Passed The PR appears limited to the CI workflow change needed for the linked E2E gating fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly states the CI change to run E2E on PRs touching E2E paths.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8339b973-6c18-41aa-8476-44eb10c2c827

📥 Commits

Reviewing files that changed from the base of the PR and between 9eff372 and 5af9b46.

📒 Files selected for processing (1)
  • .github/workflows/pr.yml

Comment thread .github/workflows/pr.yml
Comment thread .github/workflows/pr.yml
Comment thread .github/workflows/pr.yml Outdated
Use merge-base diffs so base-branch drift does not false-trigger E2E,
fail the detector when git diff cannot compute the PR range, and pin
least-privilege contents:read on both the detector and reusable E2E
workflow.
@innocarpe

Copy link
Copy Markdown
Contributor Author

Sync update (2fd4985cb)

Address CodeRabbit on E2E path detection: merge-base diff (no base-branch drift false triggers), fail when git diff cannot compute the range, least-privilege contents:read on detector + e2e reusable workflow.

@nwparker

Copy link
Copy Markdown
Contributor

Picked this up in #11131 — thanks @innocarpe, the diagnosis here was right and the path-filter approach is the correct shape.

Your two commits are preserved as the base of that branch, rebased onto current main (this one was 185 commits behind and pr.yml was restructured underneath it, so it no longer applied cleanly). I added a second commit fixing two things:

  1. verify didn't depend on e2e. That job is the required check and lists its dependencies explicitly, so a failing shard still left it green — the gate reproduced the hole it was closing. Added e2e to needs and the result list, with skipped allowed since the job is path-filtered.
  2. playwright. matched no tracked file. The config is tests/playwright.config.ts, beside tests/e2e/ rather than inside it, so editing the runner config would have silently skipped E2E. Anchored at tests/playwright..

Also added a contract test so neither can regress. Closing this in favor of #11131 — full credit retained via Co-authored-by.

@nwparker nwparker closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: E2E regression tests can merge red — PR CI never runs the E2E suite

2 participants