fix(ci): skip e2e/functional tests in merge queue for irrelevant paths - #2617
Conversation
…levant The merge_group trigger had no path filter and the relevance-check step only ran for pull_request_target events, so every PR entering the merge queue ran the full e2e and functional test suites regardless of what files changed. Extend the relevance check to also run on merge_group events, using the compare API with the merge group base/head SHAs to determine changed files. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoCI: Skip e2e/functional tests in merge queue for irrelevant changes Description
Diagram
High-Level Assessment
Files changed (2)
|
Site previewPreview: https://b481b874-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 4:38 PM UTC · Completed 4:48 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
ReviewFindingsHigh
The prior review's medium-severity edge-case findings (compare API truncation at 300 files) have been resolved — the new commit adds a Previous runReviewFindingsHigh
Medium
Labels: PR modifies CI workflow files for e2e and functional test pipelines. Previous runReviewFindingsHigh
Medium
Previous run (2)ReviewFindingsHigh
Medium
Labels: PR modifies CI workflow files for e2e and functional test pipelines. |
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| } | ||
| if [ "$EVENT_NAME" = "merge_group" ]; then |
There was a problem hiding this comment.
[medium] edge-case
The GitHub compare API returns at most 300 files and silently truncates beyond that. Unlike the pulls/files endpoint (which supports --paginate), the compare endpoint has no pagination. If a merge group accumulates more than 300 changed files, the relevance grep could incorrectly skip tests.
Suggested fix: After fetching the compare response, check if the returned file count equals 300 (the API cap). If so, assume truncation and set relevant=true as a precaution.
There was a problem hiding this comment.
Good catch. The compare API does cap at 300 files with no pagination. Added a fail-open guard in 885e295 — if we get >= 300 files back, we assume truncation and run tests.
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| } | ||
| if [ "$EVENT_NAME" = "merge_group" ]; then |
There was a problem hiding this comment.
[medium] edge-case
Same compare API truncation issue as e2e.yml. The merge group branch has no safeguard against the 300-file limit, potentially causing silent test skipping for large merge groups.
Suggested fix: Same fix as e2e.yml: check if the returned file count equals the 300-file API cap and default to relevant=true if so.
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:17 PM UTC · Completed 6:28 PM UTC |
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| } | ||
| if [ "$EVENT_NAME" = "merge_group" ]; then |
There was a problem hiding this comment.
[medium] edge-case
The GitHub compare API (/repos/{owner}/{repo}/compare/{base}...{head}) returns at most 300 files in the files array and silently truncates beyond that. Unlike the pulls/files endpoint (which supports --paginate), the compare endpoint has no pagination. If a merge group accumulates more than 300 changed files, the relevance grep could incorrectly determine no relevant files changed, causing tests to be silently skipped.
Suggested fix: After fetching the compare response, check if the returned file count equals 300 (the API cap). If so, assume truncation and set relevant=true as a precaution, matching the fail-open pattern already used for API errors.
| echo "relevant=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| } | ||
| if [ "$EVENT_NAME" = "merge_group" ]; then |
There was a problem hiding this comment.
[medium] edge-case
Same compare API truncation issue as e2e.yml. The merge group branch uses the compare API which caps the files array at 300 entries without pagination support, potentially causing silent test skipping for large merge groups.
Suggested fix: Same fix as e2e.yml: check if the returned file count equals the 300-file API cap and default to relevant=true if so.
The GitHub compare API caps at 300 files with no pagination support. Add a check after fetching merge group files: if the count is >= 300, assume possible truncation and run tests as a precaution, matching the existing fail-open pattern for API errors. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:53 PM UTC · Completed 9:04 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 2:03 PM UTC · Completed 2:09 PM UTC |
Retro: PR #2617 — CI merge queue path filter fixWhat happenedA human-authored PR by
Human reviewer Review quality assessmentPositive: The review agent's truncation finding was a genuine catch. The Compare API's 300-file cap is a real limitation with no pagination support, and the fail-open guard the author added was the correct remediation. This demonstrates strong review quality on infrastructure/CI changes. Negative: Three Existing issue coverageAll identified improvement areas are already tracked by open issues:
No new proposals are warranted. Resolving the existing issues (particularly #1500, #1551, and #1583) would have reduced this PR's review cycle from 3 rounds to 1. |
Summary
merge_grouptrigger on e2e.yml and functional-tests.yml had no path filter, and the relevance-check step only ran forpull_request_targeteventslint.yml)merge_groupevents, using the compare API with the merge group base/head SHAs to determine changed filesTest plan
*.go,eval/**) should still run tests in the merge queue🤖 Generated with Claude Code