-
Notifications
You must be signed in to change notification settings - Fork 37
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
ci(levm): silence messages #1837
base: main
Are you sure you want to change the base?
Conversation
|
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
Summary: All EF-TESTS pass. |
fc19d04
to
b7890a0
Compare
@@ -2,7 +2,7 @@ name: Benchmark LEVM vs REVM in PR | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this file not in ci_levm? That would be more consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because one file has the Integration Test
job, making it a required check
.
And the ci_bench_levm_in_pr.yaml
only modifies the github comments.
The logic was separated in case of an external contribution, if we send the PR comment inside the Integration Test
job in order to avoid repetition, the job will fail because the external contributor doesn't have access to the TOKENs
|
||
- name: Check Regression | ||
run: | | ||
if grep -q "No differences found" diff.md; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to check that levm has a regresion (levm vs levm), but that levm is equal to revm. So this check will fail until there is feature parity between revm and levm. Is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have to check against main, i will start with it, maybe it's too cumbersome for now. When LEVM is used as default we would definitely want the (levm vs levm) regression check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks it's done in commit e91040b. I save the daily artifact and use it for the comparison/check.
.github/workflows/ci_levm.yaml
Outdated
# Check we don't have a regression | ||
hive-test-check: | ||
# "Integration Test" is a required check, don't change the name | ||
name: Integration Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two integration test, can we do something like this instead? https://github.com/lambdaclass/ethrex/blob/main/.github/workflows/ci_l1.yaml#L207-L226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5f3a652
@@ -0,0 +1,100 @@ | |||
name: Run Hive Tests | |||
|
|||
on: workflow_call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to have an input
here that is vm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit e91040b. The inputs
let us create a daily
artifact. I'm using that artifact for comparisons.
.github/workflows/ci_levm.yaml
Outdated
@@ -58,6 +58,49 @@ jobs: | |||
echo "Percentage is not 100%." | |||
exit 1 | |||
fi | |||
|
|||
hive-report-creation: | |||
uses: ./.github/workflows/common_hive_reports.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since these are ran daily, can't we take the results from the last (levm) daily run and compare it to the current run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks it's done in commit e91040b. We would have to merge it to fully test it
fdada83
to
960302f
Compare
Motivation
We would like to suppress certain messages and run the hive tests in the
ci_levm.yaml
file.Description
Silence LEVM EF-TESTS messages if all of them pass.
100%
100%
All EF-TESTS pass.
if the bug is fixed within the PR.Silence LEVM Hive Tests messages if no differences were found with respect to revm.
do
find differences.