ci(l1,l2): add a workflow for testing both L1 & L2 dev #5261
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR separates the L1 & L2 dev integration testing into its own dedicated workflow, removing it from the main L2 workflow to enable independent testing of L1 dev changes.
- Creates a new standalone workflow
.github/workflows/pr-main_l1_l2_dev.yamlfor testing L1 & L2 dev together - Removes the
integration-test-l2-devjob from the existing L2 workflow - Updates the
all-testsjob dependencies to reflect the removed integration test
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/pr-main_l2.yaml |
Removes the integration-test-l2-dev job and updates the all-tests job dependencies |
.github/workflows/pr-main_l1_l2_dev.yaml |
Adds new workflow for testing L1 & L2 dev integration with ethrex running in dev mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/pr-main_l2.yaml
Outdated
| ] | ||
| [detect-changes, integration-test, state-diff-test, integration-test-tdx] | ||
| # Make sure this job runs even if the previous jobs failed or were skipped | ||
| 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' }} |
There was a problem hiding this comment.
The if condition still references needs.integration-test-l2-dev.result, but integration-test-l2-dev has been removed from the needs array. This will cause the workflow to fail because it references a non-existent dependency. Remove && needs.integration-test-l2-dev.result != 'skipped' from this condition.
| 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' }} | |
| 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' }} |
| - name: Show ENVs | ||
| run: | | ||
| cat .env |
There was a problem hiding this comment.
This step attempts to display the contents of .env file, but when using --dev mode (as in line 42), no .env file is created. The --dev flag handles configuration internally without generating an .env file. This step will fail with "No such file or directory". Consider removing this step or checking if the file exists before attempting to display it.
| cat .env | |
| if [ -f .env ]; then cat .env; else echo ".env file does not exist"; fi |
| cd crates/l2 | ||
| tail -n 100 -f /tmp/ethrex.log & | ||
| tail -n 100 -f /tmp/prover.log & | ||
| export $(shell cat .env | xargs); \ |
There was a problem hiding this comment.
The syntax $(shell cat .env | xargs) is Makefile syntax, not valid Bash syntax. In Bash, this should be $(cat .env 2>/dev/null | xargs) or similar. Additionally, this references a .env file that doesn't exist when using --dev mode. Since the --dev flag handles all configuration internally, this export statement is likely unnecessary and will cause the workflow to fail.
| export $(shell cat .env | xargs); \ |
| @@ -0,0 +1,77 @@ | |||
| name: L1 & L2 Dev | |||
There was a problem hiding this comment.
This is clearly an L2 tests, so I would call it L2 Dev
| @@ -0,0 +1,77 @@ | |||
| name: L2 Dev | |||
There was a problem hiding this comment.
We should document that this is used on L1 to make sure --dev works correctly
Motivation
L1 dev was not being tested independently of any changes in the L2.
Description
The integration test that aims to test ethrex L2 dev is suited for fulfilling this purpose.