Rework running workflows & merge queue #820
Conversation
📝 WalkthroughSummary by CodeRabbitChores
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughConsolidates multiple per-suite GitHub Actions workflows into a new matrix-based workflow at .github/workflows/vectors-test-suites.yml and removes several individual vector workflows (.github/workflows/vectors-jam-conformance-070.yml, vectors-jam-conformance-071.yml, vectors-w3f-070.yml, vectors-w3f-davxy-070.yml, vectors-w3f-davxy-071.yml, vectors-w3f-davxy-072.yml). Adds or documents the merge_group trigger in build workflows (.github/workflows/build-qa.yml and comments in build-docker.yml and utils-benchmarks.yml). No changes to exported/public code entities. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build-docker.yml(1 hunks).github/workflows/build-qa.yml(1 hunks).github/workflows/utils-benchmarks.yml(1 hunks).github/workflows/vectors-jam-conformance-070.yml(0 hunks).github/workflows/vectors-jam-conformance-071.yml(0 hunks).github/workflows/vectors-test-suites.yml(1 hunks).github/workflows/vectors-w3f-070.yml(0 hunks).github/workflows/vectors-w3f-davxy-070.yml(0 hunks).github/workflows/vectors-w3f-davxy-071.yml(0 hunks).github/workflows/vectors-w3f-davxy-072.yml(0 hunks)
💤 Files with no reviewable changes (6)
- .github/workflows/vectors-w3f-070.yml
- .github/workflows/vectors-jam-conformance-071.yml
- .github/workflows/vectors-w3f-davxy-070.yml
- .github/workflows/vectors-w3f-davxy-071.yml
- .github/workflows/vectors-w3f-davxy-072.yml
- .github/workflows/vectors-jam-conformance-070.yml
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: w3f-davxy-0.7.2
- GitHub Check: w3f-davxy-0.7.0
- GitHub Check: w3f-davxy-0.7.1
- GitHub Check: benchmarks (22.x)
- GitHub Check: jam-e2e (22.x)
- GitHub Check: test (22.x)
- GitHub Check: e2e (22.x)
🔇 Additional comments (6)
.github/workflows/build-docker.yml (1)
8-8: Intentional skip of merge queue execution—LGTM.Commenting out the merge_group trigger appropriately skips the Docker build in the merge queue, aligning with the PR objective to reduce merge queue workload.
.github/workflows/utils-benchmarks.yml (1)
9-9: Intentional skip of merge queue execution—LGTM.Commenting out the merge_group trigger appropriately avoids re-running benchmarks in the merge queue, reducing unnecessary CI overhead and aligning with the PR's goal to speed up merge queue validation.
.github/workflows/build-qa.yml (1)
8-8: Merge queue trigger correctly activates required test job.Adding the merge_group trigger directly addresses the PR objective to fix the required
testjob not running in the merge queue. This ensures all GP_VERSION variants (0.7.0, 0.7.1, 0.7.2) are validated before merge..github/workflows/vectors-test-suites.yml (3)
7-7: Clarify merge_group handling in concurrency group.Line 7 comments out the merge_group trigger (intentionally skipping this workflow in merge queue), but line 10's concurrency group still references
github.event.merge_group.head_ref. While this is functionally safe (the OR chain falls back togithub.ref), it's semantically confusing to reference a context that can never be populated.Recommendation: If merge_group is intentionally excluded, simplify line 10 to remove the dead reference:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.merge_group.head_ref || github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}Alternatively, if the workflow should run in merge queue, uncomment line 7.
Also applies to: 10-10
60-94: Step structure and workflow execution—LGTM.The job structure is well-organized with proper caching, conditional checkout, and failure artifact uploads. The matrix interpolation across all steps is consistent and correct.
14-14: Verify that load-test-ref.sh script exists and properly sets TEST_VECTORS_REF.Line 14 initializes TEST_VECTORS_REF to a placeholder hash, expecting it to be overwritten by the
load-test-ref.shscript at line 63. The script correctly exports TEST_VECTORS_REF to GITHUB_ENV using the actual test vector reference (6129da00879bbb2831ef01588604fbade1ec197a). If this script doesn't run or fails to export the variable, the workflow will cache and checkout using the wrong test vector reference, causing test failures. Verify the script remains executable and the workflow step continues to be executed before cache operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/vectors-test-suites.yml (1)
49-79: Consider standardizing artifact naming for consistency.Artifact names use inconsistent prefixes:
conformance-,w3f-, and abbreviateddavxy-(instead of fullw3f-davxy-). While functionally unique and correct, standardizing to full suite names (e.g.,w3f-davxy-070-resultsinstead ofdavxy-070-results) would improve clarity when scanning artifact lists.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/vectors-test-suites.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e (22.x)
- GitHub Check: test (22.x)
- GitHub Check: jam-e2e (22.x)
🔇 Additional comments (3)
.github/workflows/vectors-test-suites.yml (3)
49-79: Artifact naming issue from past review has been resolved.Each matrix entry now has a unique artifact name (e.g.,
davxy-070-results,davxy-071-results,davxy-072-resultsfor w3f-davxy variants), addressing the earlier concern about w3f-davxy-071 and w3f-davxy-072 overwriting the same artifact. This ensures all test results are preserved across parallel runs.
7-7: Verify merge_group intent aligns with PR objectives.Line 7 comments out
merge_groupto skip this workflow in the merge queue, reasoning that it's slow. However, the PR description states "Fixes the requiredtestjob not being run in the merge queue." Clarify whether this vector test workflow is intentionally excluded from merge queue checks, or if a differenttestjob handles that requirement.Also applies to: 49-79
10-10: Verify parallel matrix execution and cache behavior.The workflow runs 6 matrix jobs in parallel, all sharing the same cache key
jam-test-vectors-${{ env.TEST_VECTORS_REF }}. Confirm that:
- The shared cache key is correct (i.e., all test suites reside in a single
fluffylabs/test-vectorsrepository).- No race conditions or cache corruption occur when multiple jobs restore concurrently.
- The
GP_VERSIONenvironment variable (line 82) correctly isolates test execution across matrix items.Also applies to: 81-118
View all
Benchmarks summary: 82/83 OK ❌ |
Closes #710
Fixes the required
testjob not being run in merge queue.Also reworks the test vectors to use single workflow file and a matrix.
On top, I've decided to run less stuff in merge queue to speed up merging. We risk bugs that are introduced by auto-merging parallel PRs, but I think it should be super rare, and it's acceptable for dev speed.