Skip to content

ci(l1): have failed tests output on the console #3150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Jun 20, 2025

Conversation

cdiielsi
Copy link
Contributor

@cdiielsi cdiielsi commented Jun 12, 2025

Motivation

The LEVM CI workflow in pr-main_levm.yaml that runs EF state tests should fail with an exit code if a test fails.

Description
This pr introduces a new EFTestRunnerError::TestsFailed error to use when there's a report of a test failing. This error is thrown under the summary flag, which is the one used in the target the CI job executes: make run-evm-ef-tests-ci. So whenever there is any failing tests, the introduced code should print the EFTestReport and then finish with the EFTestRunnerError::TestsFailed error.

Note: The summary flag is used in other targets as well, so the previously described behavior is being implemented for other targets too.

The ef-test-main job in pr-main_levm has also been refactored, I dropped steps "Check EF-TESTS from Paris to Prague is 100%" and "Check EF-TESTS status is 100%" since now in the case any test fails, the CI job exits with an error code and outputs the failing tests in the console.

In this pr there are some commits with a hardcoded error with the intentions of having the LEVM CI workflow fail on purpose and check the console output is the one expected. Here is a failing workflow execution under this circumstances to see. (The underscore line above "Failed tests" was removed on a later commit.)

Closes #2887

Copy link

github-actions bot commented Jun 13, 2025

Lines of code report

Total lines added: 28
Total lines removed: 0
Total lines changed: 28

Detailed view
+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/report.rs             | 834   | +3   |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/runner/levm_runner.rs | 387   | +5   |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/runner/mod.rs         | 259   | +10  |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/runner/revm_runner.rs | 564   | +10  |
+-------------------------------------------------+-------+------+

@cdiielsi
Copy link
Contributor Author

Current commit has EF Test Check failing and printing all failed tests on the console. I implemented this directly on the levm_runner and I hardcoded an error on the test set up so we could see if the job fails as expected.

Since the command run-evm-ef-tests-ci uses flag summary to run the tests, I implemented some code to have the test runner fail after printing failed test when thais flag is set.
I'm not sure if I should introduce a new flag for this behavior or if I should leave it as it is. Thoughts?

I also introduced a new EFTestRunnerError::CITestsFailed to use in this case in particular because I thought it was more appropiate than throwing a panic!

@cdiielsi cdiielsi marked this pull request as ready for review June 17, 2025 12:25
@cdiielsi cdiielsi requested a review from a team as a code owner June 17, 2025 12:25
@mpaulucci mpaulucci requested a review from Copilot June 17, 2025 12:36
@cdiielsi cdiielsi changed the title feat(l1): have failed tests output on the console ci(l1): have failed tests output on the console Jun 17, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Ensure the CI job exits with a failure code when tests fail and prints out which tests failed.

  • Add a new CITestsFailed error variant and logic to print failing tests in the summary phase
  • Update REVM and LEVM runners to handle or skip the new CITestsFailed variant
  • Remove outdated percentage-check steps from the GitHub Actions workflow

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
cmd/ef_tests/state/runner/mod.rs Added CITestsFailed variant, summary logic to print failures, and print_failed_tests function
cmd/ef_tests/state/runner/revm_runner.rs Updated match to skip CITestsFailed in REVM runner
cmd/ef_tests/state/runner/levm_runner.rs Updated match to skip CITestsFailed and changed chain_id from 1 to 10
.github/workflows/pr-main_levm.yaml Removed temporary EF-TESTS percentage check steps
Comments suppressed due to low confidence (3)

cmd/ef_tests/state/runner/levm_runner.rs:180

  • The chain_id was changed to 10 (Optimism) but L1 Mainnet tests expect chain_id 1; revert to 1 or parameterize the chain ID appropriately.
            chain_id: U256::from(10),

cmd/ef_tests/state/runner/levm_runner.rs:43

  • The Err(EFTestRunnerError::CITestsFailed) pattern is unreachable because run_ef_test_tx never returns that variant; consider removing it to simplify the match.
                Ok(_) | Err(EFTestRunnerError::CITestsFailed) => continue, // An EFTestRunnerError::CITestsFailed can't happen at this point.

cmd/ef_tests/state/runner/revm_runner.rs:462

  • The Err(EFTestRunnerError::CITestsFailed) arm is unreachable in this context since re_run_failed_ef_test does not return that variant; removing it avoids confusion.
                Ok(_) | Err(EFTestRunnerError::CITestsFailed) => continue, // An EFTestRunnerError::CITestsFailed can't happen at this point.

Copy link
Contributor

@JereSalo JereSalo left a comment

Choose a reason for hiding this comment

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

Great!
I'd maybe create an issue with the task to refactor the tests runner module and make it nicer. We could do much better in terms of code quality, and that would help us make changes in the future that can help us debug faster and get more information easily.

Edit: I approved changes but ofc we have to change the chain_id to the previous value so that it doesn't break :) Thanks for showing us how it would look like!

@mpaulucci mpaulucci added this pull request to the merge queue Jun 20, 2025
Merged via the queue into main with commit 9c03110 Jun 20, 2025
31 checks passed
@mpaulucci mpaulucci deleted the fail_ci_when_levm_tests_fail branch June 20, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Levm CI job should fail if state tests fail
3 participants