Skip to content
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

Testool fix from Scroll #1816

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Testool fix from Scroll #1816

wants to merge 3 commits into from

Conversation

curryrasul
Copy link
Member

Description

This PR adds fixes from Scroll/testool - scroll-tech#1235.

Initially I wanted to do that by cherry-pick, but it added much more here - so I did it manually.

@curryrasul curryrasul self-assigned this May 9, 2024
@@ -10,6 +10,13 @@ path="tests/src/GeneralStateTestsFiller/**/*"
max_gas = 0
max_steps = 100000

[[suite]]
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, do we need to add this @ChihChengLiang ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add this one since our TLOAD TSTORE is implemented.

@curryrasul
Copy link
Member Author

curryrasul commented May 9, 2024

Another question - do we need to do changes in codehash.txt, as it's done here ? @ChihChengLiang

Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

do we need to do changes in codehash.txt

We should add changes to codehash.txt. The content is the cached mapping of code hash to the corresponding bytecode. When we run the EIP1153 test, cached entries should be automatically generated. We check in those entries to reduce the testing overhead.

@@ -10,6 +10,13 @@ path="tests/src/GeneralStateTestsFiller/**/*"
max_gas = 0
max_steps = 100000

[[suite]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add this one since our TLOAD TSTORE is implemented.

@curryrasul
Copy link
Member Author

We should add changes to codehash.txt. The content is the cached mapping of code hash to the corresponding bytecode. When we run the EIP1153 test, cached entries should be automatically generated. We check in those entries to reduce the testing overhead.

@ChihChengLiang - We have an old version of ethereum/tests submodule, that doesn't contain Cancun tests.
I updated it for the latest version and got the error when was running both default suite and EIP1153; and I also tried to switch to the commit that's scroll locked on and it also gives me error.
The error is:

Error found not a str

@ChihChengLiang
Copy link
Collaborator

@curryrasul Can you push the update for the ethereum/test submodule?
Can you give me a guide on how to reproduce the error so I can take a look?

@curryrasul
Copy link
Member Author

@ChihChengLiang I updated the tests submodule

To reproduce the error:

  1. cd testool
  2. cargo run --release --bin testool -- --suite EIP1153
    or
    cargo run --release --bin testool

It returns:
[2024-05-15T17:44:24Z INFO testool] Using suite 'EIP1153'
[2024-05-15T17:44:24Z INFO testool] Parsing and compiling tests...
Error found not a str

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.

None yet

2 participants