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

Tests for arithmetic opcodes #1144

Closed
wants to merge 3 commits into from
Closed

Tests for arithmetic opcodes #1144

wants to merge 3 commits into from

Conversation

qbzzt
Copy link
Collaborator

@qbzzt qbzzt commented Jan 15, 2023

9156 tests. The code is ~400k bytes, but it bypasses the tests by specifying it as code: rather than CREATE[2] ing it, so it works, at least on geth.

Copy link
Contributor

@chfast chfast left a comment

Choose a reason for hiding this comment

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

image

Why do you think generating huge tests like this is any good idea? Even GitHub refused to show it.

data:
- 0x
gasLimit:
- 80000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Stop using arbitrary large gas limits.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Jan 16, 2023

Why do you think generating huge tests like this is any good idea? Even GitHub refused to show it.

Because in this case the Filler is the compiled version. The source code is actually https://github.com/ethereum/tests/blob/bdfd202b89d8b3845fad6a3e4af91d89d4c7897a/src/Templates/EachOpcode/Arith.ts, which is less than 400 lines.

Stop using arbitrary large gas limits.

Why? It makes it easier to run tests without worrying about the gas limit, when the gas limit is not part of what we test.

@chfast
Copy link
Contributor

chfast commented Jan 16, 2023

Why? It makes it easier to run tests without worrying about the gas limit, when the gas limit is not part of what we test.

Not everyone can run a test with 80M gas limit.

@chfast
Copy link
Contributor

chfast commented Jan 16, 2023

Because in this case the Filler is the compiled version. The source code is actually https://github.com/ethereum/tests/blob/bdfd202b89d8b3845fad6a3e4af91d89d4c7897a/src/Templates/EachOpcode/Arith.ts, which is less than 400 lines.

If you need additional language to generate a filler to generate a test you better contribute it to https://github.com/ethereum/execution-spec-tests.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Jan 17, 2023

If you need additional language to generate a filler to generate a test you better contribute it to https://github.com/ethereum/execution-spec-tests.

How about I get started with writing docs for how to write tests for that system? It looks similar to retesteth, but there's enough difference to justify more docs.

@qbzzt qbzzt marked this pull request as draft January 19, 2023 04:54
@winsvega
Copy link
Collaborator

yes makes sense to look into python specs if filler design is too complex.
but also Ori, I would love to have the docs on pyspecs on how to regenerate those tests with given t8n tool, and on new forks, and how to analyze their expect section.
also pyspecs need update on generated test format as it generates incomplete accounts and the fields format is incoherent with the current one we use in tests

@qbzzt
Copy link
Collaborator Author

qbzzt commented Jan 29, 2023

@winsvega: also pyspecs need update on generated test format as it generates incomplete accounts

Can you give me the name of a test where it does that and I'll try to fix it?

@winsvega
Copy link
Collaborator

Its fixed now

@qbzzt
Copy link
Collaborator Author

qbzzt commented Jan 29, 2023

I would love to have the docs on pyspecs on how to regenerate those tests with given t8n tool, and on new forks,

I added that to ethereum/execution-spec-tests#51 - you just update the evm in the path to the new one,

@winsvega
Copy link
Collaborator

winsvega commented Feb 2, 2023

Thats not very convenient when I have multiple evms on the same machine

I would need to run mv commands. Would be much nicer if I could provide the path to python script

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 2, 2023 via email

@winsvega
Copy link
Collaborator

winsvega commented Feb 3, 2023

Are you redoing this PR with pyspecs too?

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 3, 2023 via email

@qbzzt
Copy link
Collaborator Author

qbzzt commented Feb 4, 2023

@qbzzt qbzzt closed this Feb 4, 2023
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

3 participants