Skip to content

Commit db22fbc

Browse files
gianbelinchegianbelinche
authored
fix(l1,l2): make l2 integration tests and lint required (#5142)
**Motivation** The L2 CI is not running on some PRs it should. <!-- Why does this pull request exist? What are its goals? --> **Description** The problem is that the required checks `Integration Test` and `Lint` have the same name for both L1 and L2. So if one of them passes, then the other is not needed to merge. This can be a problem in the case that the L1 CI finishes before the L2 one starts, in that case the L2 CI will not run. The solution is the following: - Add `Integration Test L2` and `Lint L2` to the branch protection rules - Rename the corresponding jobs on the L2 workflow. With only those changes, we will always require the L2 workflows to run in order to merge, since we want to be able only run the L1 CI in PRs that don't touch the L2, we need a way to skip it. Note: The current way of not running a workflow doesn't work, since skipping a complete workflow with `paths:` makes the jobs abscent, what we need is for the jobs to be marked as skipped. To do this, [dorny/paths-filter@v3](https://github.com/dorny/paths-filter) action is used, which performs the same action as the `paths:` option, but as a job itself. Then with the result of this job we can add ifs to the rest of the jobs to mark if they should be run. Examples with a fork of ethrex with the branch protection already active (For Integration Test L2) - [PR with changes only in L1](gianbelinche#4): It ran L1 checks and skipped L2 ones. - [PR with changes only in L2](gianbelinche#3): It ran L2 checks and skipped L1 ones. - [PR with both L1 and L2 changes](gianbelinche#5): It ran both L1 and L2 checks. You can also check that when the PRs were merged to main, all checks ran even though the PRs skipped the checks. https://github.com/gianbelinche/ethrex/commits/main/ <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 -->
1 parent 084c899 commit db22fbc

File tree

2 files changed

+85
-24
lines changed

2 files changed

+85
-24
lines changed

‎.github/workflows/pr-main_l1.yaml‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ on:
55
merge_group:
66
pull_request:
77
branches: ["**"]
8-
paths-ignore:
9-
- "crates/l2/**" # Behind a feature flag not used in this workflow
108

119
permissions:
1210
contents: read
@@ -17,10 +15,35 @@ concurrency:
1715
cancel-in-progress: true
1816

1917
jobs:
18+
detect-changes:
19+
runs-on: ubuntu-latest
20+
outputs:
21+
run_tests: ${{ steps.finish.outputs.run_tests }}
22+
steps:
23+
- uses: actions/checkout@v4
24+
- uses: dorny/paths-filter@v3
25+
id: filter
26+
with:
27+
filters: |
28+
run_tests:
29+
- '!crates/l2/**'
30+
- name: finish
31+
id: finish
32+
run: |
33+
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
34+
echo "run_tests=true" >> "$GITHUB_OUTPUT"
35+
else
36+
echo "run_tests=${{ steps.filter.outputs.run_tests }}" >> "$GITHUB_OUTPUT"
37+
fi
38+
- name: Print result
39+
run: echo "run_tests=${{ steps.finish.outputs.run_tests }}"
40+
2041
lint:
2142
# "Lint" is a required check, don't change the name
2243
name: Lint
2344
runs-on: ubuntu-latest
45+
needs: detect-changes
46+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
2447
steps:
2548
- name: Checkout sources
2649
uses: actions/checkout@v4
@@ -61,6 +84,8 @@ jobs:
6184
test:
6285
# "Test" is a required check, don't change the name
6386
name: Test
87+
needs: detect-changes
88+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
6489
runs-on: ubuntu-latest
6590
steps:
6691
- name: Checkout sources
@@ -84,6 +109,8 @@ jobs:
84109
docker_build:
85110
name: Build Docker
86111
runs-on: ubuntu-latest
112+
needs: detect-changes
113+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
87114
steps:
88115
- uses: actions/checkout@v4
89116

@@ -105,8 +132,8 @@ jobs:
105132
run-assertoor:
106133
name: Assertoor - ${{ matrix.name }}
107134
runs-on: ubuntu-latest
108-
needs: [docker_build]
109-
if: ${{ github.event_name != 'merge_group' }}
135+
needs: [detect-changes, docker_build]
136+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && github.event_name != 'merge_group' }}
110137
strategy:
111138
fail-fast: true
112139
matrix:
@@ -147,8 +174,8 @@ jobs:
147174
run-hive:
148175
name: Hive - ${{ matrix.name }}
149176
runs-on: ubuntu-latest
150-
needs: [docker_build]
151-
if: ${{ github.event_name != 'merge_group' }}
177+
needs: [detect-changes, docker_build]
178+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && github.event_name != 'merge_group' }}
152179
strategy:
153180
fail-fast: false
154181
matrix:
@@ -254,9 +281,9 @@ jobs:
254281
# "Integration Test" is a required check, don't change the name
255282
name: Integration Test
256283
runs-on: ubuntu-latest
257-
needs: [run-assertoor, run-hive]
284+
needs: [detect-changes, run-assertoor, run-hive]
258285
# Make sure this job runs even if the previous jobs failed or were skipped
259-
if: ${{ always() && needs.run-assertoor.result != 'skipped' && needs.run-hive.result != 'skipped' }}
286+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && always() && needs.run-assertoor.result != 'skipped' && needs.run-hive.result != 'skipped' }}
260287
steps:
261288
- name: Check if any job failed
262289
run: |
@@ -273,7 +300,8 @@ jobs:
273300
reorg-tests:
274301
name: Reorg Tests
275302
runs-on: ubuntu-latest
276-
if: ${{ github.event_name != 'merge_group' }}
303+
needs: detect-changes
304+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && github.event_name != 'merge_group' }}
277305
steps:
278306
- name: Checkout sources
279307
uses: actions/checkout@v4

‎.github/workflows/pr-main_l2.yaml‎

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,6 @@ on:
44
branches: ["main"]
55
pull_request:
66
branches: ["**"]
7-
paths:
8-
- "crates/l2/**"
9-
- "fixtures/**"
10-
- "crates/blockchain/dev/**"
11-
- "crates/vm/levm/**"
12-
- ".github/workflows/pr-main_l2.yaml"
13-
- "cmd/ethrex/l2/**"
147

158
permissions:
169
contents: read
@@ -24,10 +17,40 @@ env:
2417
DOCKER_ETHREX_WORKDIR: /usr/local/bin
2518

2619
jobs:
20+
detect-changes:
21+
runs-on: ubuntu-latest
22+
outputs:
23+
run_tests: ${{ steps.finish.outputs.run_tests }}
24+
steps:
25+
- uses: actions/checkout@v4
26+
- uses: dorny/paths-filter@v3
27+
id: filter
28+
with:
29+
filters: |
30+
run_tests:
31+
- "crates/l2/**"
32+
- "fixtures/**"
33+
- "crates/blockchain/dev/**"
34+
- "crates/vm/levm/**"
35+
- ".github/workflows/pr-main_l2.yaml"
36+
- "cmd/ethrex/l2/**"
37+
- name: finish
38+
id: finish
39+
run: |
40+
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
41+
echo "run_tests=true" >> "$GITHUB_OUTPUT"
42+
else
43+
echo "run_tests=${{ steps.filter.outputs.run_tests }}" >> "$GITHUB_OUTPUT"
44+
fi
45+
- name: Print result
46+
run: echo "run_tests=${{ steps.finish.outputs.run_tests }}"
47+
2748
lint:
28-
# "Lint" is a required check, don't change the name
29-
name: Lint
49+
# "Lint L2" is a required check, don't change the name
50+
name: Lint L2
3051
runs-on: ubuntu-latest
52+
needs: detect-changes
53+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
3154
steps:
3255
- name: Checkout sources
3356
uses: actions/checkout@v4
@@ -69,6 +92,8 @@ jobs:
6992
build-docker:
7093
name: Build docker image
7194
runs-on: ubuntu-latest
95+
needs: detect-changes
96+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
7297
steps:
7398
- name: Checkout sources
7499
uses: actions/checkout@v4
@@ -94,6 +119,8 @@ jobs:
94119
build-docker-l2:
95120
name: Build docker image L2
96121
runs-on: ubuntu-latest
122+
needs: detect-changes
123+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
97124
steps:
98125
- name: Checkout sources
99126
uses: actions/checkout@v4
@@ -119,7 +146,8 @@ jobs:
119146
integration-test:
120147
name: Integration Test - ${{ matrix.name }}
121148
runs-on: ubuntu-latest
122-
needs: [build-docker, build-docker-l2]
149+
needs: [detect-changes, build-docker, build-docker-l2]
150+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
123151
strategy:
124152
matrix:
125153
include:
@@ -309,7 +337,8 @@ jobs:
309337
integration-test-tdx:
310338
name: Integration Test - TDX
311339
runs-on: ubuntu-latest
312-
needs: [build-docker, build-docker-l2]
340+
needs: [detect-changes, build-docker, build-docker-l2]
341+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
313342
steps:
314343
- name: Checkout sources
315344
uses: actions/checkout@v4
@@ -416,7 +445,8 @@ jobs:
416445
state-diff-test:
417446
name: State Reconstruction Tests
418447
runs-on: ubuntu-latest
419-
needs: [build-docker, build-docker-l2]
448+
needs: [detect-changes, build-docker, build-docker-l2]
449+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
420450
steps:
421451
- name: Checkout sources
422452
uses: actions/checkout@v4
@@ -477,6 +507,8 @@ jobs:
477507
integration-test-l2-dev:
478508
name: Integration Test - L2 Dev
479509
runs-on: ubuntu-latest
510+
needs: detect-changes
511+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' }}
480512
steps:
481513
- name: Checkout sources
482514
uses: actions/checkout@v4
@@ -531,18 +563,19 @@ jobs:
531563
532564
# The purpose of this job is to add it as a required check in GitHub so that we don't have to add every individual job as a required check
533565
all-tests:
534-
# "Integration Test" is a required check, don't change the name
535-
name: Integration Test
566+
# "Integration Test L2" is a required check, don't change the name
567+
name: Integration Test L2
536568
runs-on: ubuntu-latest
537569
needs:
538570
[
571+
detect-changes,
539572
integration-test,
540573
state-diff-test,
541574
integration-test-tdx,
542575
integration-test-l2-dev,
543576
]
544577
# Make sure this job runs even if the previous jobs failed or were skipped
545-
if: ${{ always() && needs.integration-test.result != 'skipped' && needs.state-diff-test.result != 'skipped' && needs.integration-test-tdx.result != 'skipped' && needs.integration-test-l2-dev.result != 'skipped' }}
578+
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && always() && needs.integration-test.result != 'skipped' && needs.state-diff-test.result != 'skipped' && needs.integration-test-tdx.result != 'skipped' && needs.integration-test-l2-dev.result != 'skipped' }}
546579
steps:
547580
- name: Check if any job failed
548581
run: |

0 commit comments

Comments
 (0)