Conversation
chfast
left a comment
There was a problem hiding this comment.
Can you give it any better name? The "opcode diff places" has no meaning.
|
it translates as opcode in different (logical) places |
marioevz
left a comment
There was a problem hiding this comment.
A few suggestions after a quick review. I haven't checked files in the ./scenarios/ scenarios/ folder, but will do on the re-review.
b1af590 to
ff2eeff
Compare
b28f2ef to
f7de36c
Compare
5d3dd84 to
940cb3e
Compare
|
Since it is unreasonable to have all scenarios in one transaction (millions of gas and huge debug trace) so here what it does. so if we test something like selfbalance == actual selfbalance, we create bytecode then scenarios will make sure that this bytecode return value is equal to scenario expected selfbalance (which is calculated at scenario code) for each scenario situation. like when we call delegatecall->program, delegatecall->call->program then we can add a scenario of eof code. and it will verify program value inside eof code. this was one of the first ethereum tests. a simple verification that a bytecode like add(1,1) really returns 2 in all contexts. 1 coverage line is lost because it's no longer a state test, besides that line reffers to pre state check in test. Also original tests were a little faulty, some cases didn't do what they were intended to do. |
marioevz
left a comment
There was a problem hiding this comment.
I think there are a lot of anti-patterns used here, each program can be a separate python test file to make them simpler and more atomic.
The problem with putting so many verifications into a single test is that it's going to be difficult to client developers to debug in the case they find issues with one of these tests.
56069b2 to
2e849b3
Compare
|
Missing coverage 3 lines are |
rebase
marioevz
left a comment
There was a problem hiding this comment.
I've rebased and deleted the static test files that this PR supersedes.
I also disabled this test in executed because it requires accurate value of Op.GASPRICE which is unpredictable in devnets (we need a way to pass this to the tests somehow perhaps via a fixture).
|
Verified the coverage files:
|
I am aming ro make it execute compatible. |
* test scenarios * format with ruff * address some comments * fix invalid opcodes rebase * generate 1 test block per scenario per program * fix style * fix all_opcodes test * refactor with test program class * fix(tests): Delete updated static tests * fix(tests): skip on execute due to `Op.GASPRICE` --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
* test scenarios * format with ruff * address some comments * fix invalid opcodes rebase * generate 1 test block per scenario per program * fix style * fix all_opcodes test * refactor with test program class * fix(tests): Delete updated static tests * fix(tests): skip on execute due to `Op.GASPRICE` --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
🗒️ Description
Conversion of opcode diff places tests by ori.
A test defines series of test scenarios that are run on each parametrized opcode sequence.
Then we check if the sequence worked as expected in a given scenario.
I think this is a powerful method to template test any new given opcode.
we already have pre defined scenarios. then we just add one more parameter with what we want to test, and it will be covered on all the cases automatically.
Cases can be like:
callcode->staticcall-> [opcode]
create2-> [opcode]
[opcode] -> revert
check it out. so we can define opcode programs and scenarios. then the test will put each opcode program in each scenario and verify that it's result is the same (perhaps result will be complex depending on context and fork)
the idea is so far to have a template test and then we can easily just add opcode programms and it will be run in all crazy combinations.
likce call delegate call suicide revert and so on
This is still WIP.
🔗 Related Issues
#184
ethereum/tests#1490
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.