E2E o1VM testing with cached artifacts. #2700
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2700 +/- ##
==========================================
- Coverage 72.52% 72.51% -0.02%
==========================================
Files 247 247
Lines 57706 57707 +1
==========================================
- Hits 41851 41845 -6
- Misses 15855 15862 +7 ☔ View full report in Codecov by Sentry. |
| workflow_dispatch: | ||
| pull_request: | ||
| paths: | ||
| - "o1vm/**" |
There was a problem hiding this comment.
When we do modify other parts of the code, like poly-commitment and co, we should also run it.
| echo "Preparing the file system..." | ||
| echo "" | ||
| zip -r -q o1vm-e2e-testing-cache.zip op-program-db-for-latest-l2-block \ | ||
| env-for-latest-l2-block.sh \ |
There was a problem hiding this comment.
Nit: could we instead use the block number in the name?
I know it is annoying to rebuild the docker image, so maybe later if one day we change the data.
| @@ -10,6 +10,10 @@ set -u | |||
|
|
|||
| source $FILENAME | |||
|
|
|||
| ./run-op-program.sh | |||
| ./run-cannon.sh | |||
| if [ -d "op-program-db-for-latest-l2-block" ] && [ -f "env-for-latest-l2-block.sh" ] && [ -f "snapshot-state-3000000.json" ] && [ -f "meta.json" ]; then | |||
There was a problem hiding this comment.
I'm a bit against this. Having this condition doesn't mean we don't want to get the latest data. I would keep this file simple.
|
|
||
| cargo run --bin ${BINARY_FLAVOR} \ | ||
| if [ -d "op-program-db-for-latest-l2-block" ] && [ -f "snapshot-state-3000000.json" ]; then |
There was a problem hiding this comment.
Same as above. I would simply change how the env file is generated in the CI (?) to keep it simple.
| export L2_RPC="http://localhost:8765" | ||
| fi | ||
|
|
||
| RUST_BACKTRACE=full cargo run --bin ${BINARY_FLAVOR} \ |
There was a problem hiding this comment.
I would remove RUST_BACKTRACE=full.
| @@ -1286,7 +1286,10 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> Env<Fp, PreImageOracle> { | |||
| let insn = self.get_opcode().unwrap(); | |||
|
|
|||
| // Approximate instruction per seconds | |||
| let how_many_steps = step as usize - start.step; | |||
| let how_many_steps = (step as usize) | |||
There was a problem hiding this comment.
You still need this even with the latest changes on master? 🤔
Closes #2693