ci(l1): enable blockchain ef tests to be run with levm #2440
Conversation
Lines of code reportTotal lines added: Detailed view |
JulianVentura
changed the title
mpaulucci
changed the title
| // Discard this test | ||
| continue; | ||
| } | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
instead of allow dead code, you can do #[cfg(not(feature = "levm"))] right?
There was a problem hiding this comment.
Yes, that's an alternative, but since the feature flags usually don't go well with LSPs, I tried to use them just on the harness.
I can make the change if you want, just let me know!
There was a problem hiding this comment.
I'd prefer to use actual feature flags, I haven't had any problems with LSP. But if you feel strongly about this you can leave it as is.
There was a problem hiding this comment.
The risk of allowing dead code is that it might become actual dead code add some point. And the fact that it is not clear why it was set as dead code.
There was a problem hiding this comment.
I agree with using the feature flag
There was a problem hiding this comment.
No problem! I'll change it, I'm not strongly about this.
| } | ||
| } | ||
|
|
||
| pub fn parse_and_execute_with_filter(path: &Path, evm: EvmEngine, skipped_tests: &[&str]) { |
There was a problem hiding this comment.
nit: instead of having two functions, I would just have parse_and_execute , with an optional third argument.
There was a problem hiding this comment.
I went for this option because it's easier to see in the logs which VM is being executed
Yep, it's a good idea
| // Discard this test | ||
| continue; | ||
| } | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
I agree with using the feature flag
Motivation
We want to run the Ethereum Foundation blockchain tests with LEVM. Currently, we are only running them with REVM.
Description
This PR modifies the EF tests runner so it executes the EF tests with both VMs. The implementation combines both executions on the same command
cargo test, but could be easily modified to include a feature flag to separate both executions if that's desired.