chore: self-contained revert tests, contract reorg, and failure analysis#3033
chore: self-contained revert tests, contract reorg, and failure analysis#3033mojtaba-esk wants to merge 11 commits intomainfrom
Conversation
…t-reorg,-and-failure-analysis
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3033 +/- ##
==========================================
- Coverage 58.35% 58.32% -0.04%
==========================================
Files 2079 2079
Lines 171885 171662 -223
==========================================
- Hits 100304 100114 -190
+ Misses 62645 62626 -19
+ Partials 8936 8922 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
left a comment
There was a problem hiding this comment.
Left a couple of comments about the assertion logic and missing coverage on panic/other failure modes.
Great to see irrelevant tests getting removed 🙌
| >> {"jsonrpc":"2.0","id":1,"method":"eth_call","params":[{"from":"0x0000000000000000000000000000000000000000","gas":"0x186a0","input":"0x01","to":"0x0ee3ab1371c93e7c0c281cc0c2107cdebc8b1930"},"latest"]} | ||
| // Self-contained: script deploys reverter contract; runner substitutes __REVERTER__ with its address. | ||
| // Expects eth_call to return JSON-RPC error code 3 with ABI-encoded Error("user error"). | ||
| >> {"jsonrpc":"2.0","id":1,"method":"eth_call","params":[{"from":"0x0000000000000000000000000000000000000000","gas":"0x186a0","input":"0x01","to":"__REVERTER__"},"latest"]} |
There was a problem hiding this comment.
This now only covers the error path; we are missing coverage for panic now that *-panic.io is removed?
There was a problem hiding this comment.
Good point!
we were only covering the error path. I've restored panic coverage.
| // speconly: client response is only checked for schema validity. | ||
| >> {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[{"from":"0x0102030000000000000000000000000000000000","input":"0x01","to":"0x0ee3ab1371c93e7c0c281cc0c2107cdebc8b1930"}]} | ||
| // Self-contained: script deploys reverter; runner substitutes __REVERTER__. Expects eth_estimateGas to return error (revert). | ||
| >> {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[{"from":"0x0000000000000000000000000000000000000000","input":"0x01","to":"__REVERTER__"}]} |
There was a problem hiding this comment.
Similar to eth_call comment, i think now that estimate-failed-call.io is removed we are missing tests for panic and other revert/failure modes.
There was a problem hiding this comment.
Agreed. I've added the panic fixture.
| | eth_simulateV1 | ethSimulate-transfer-over-BlockStateCalls.iox | FAIL | Sei | Spec-only check failed | response kind mismatch: expected result=true error=false, actual result=false error=true | | ||
| | eth_simulateV1 | ethSimulate-two-blocks-with-complete-eth-sends.iox | FAIL | Sei | Spec-only check failed | response kind mismatch: expected result=true error=false, actual result=false error=true | | ||
| | eth_simulateV1 | ethSimulate-use-as-many-features-as-possible.iox | FAIL | Sei | Spec-only check failed | response kind mismatch: expected result=true error=false, actual result=false error=true | | ||
| | eth_syncing | check-syncing.iox | FAIL | Sei | Spec-only check failed | response kind mismatch: expected result=true error=false, actual result=false error=true | |
There was a problem hiding this comment.
Oh I see we are removing simualatev1 tests.
…t-reorg,-and-failure-analysis
…t-reorg,-and-failure-analysis
…t-reorg,-and-failure-analysis
masih
left a comment
There was a problem hiding this comment.
Apart from || true LGTM 🙌
…t-reorg,-and-failure-analysis
…t-reorg,-and-failure-analysis
bdchatham
left a comment
There was a problem hiding this comment.
A few minor concerns with files moved and slight inconsistencies between FAILED_TEST_ANALYSIS and RPC_IO_README but non-blockers
There was a problem hiding this comment.
Was this move intention? I'm guessing yes so that the test runner doesn't find it
monty-sei
left a comment
There was a problem hiding this comment.
No noticeable issues stand out. All good on my end ✅
…t-reorg,-and-failure-analysis
Describe your changes and provide context
This PR proposes a number of improvements on EVM RPC tests. The failing tests' fixtures are manually analyzed and updated to have Ethereum-expected behavior with adding the required steps and tags.
Updated the README and analysis doc describe the suite, removed tests, and failure breakdown.
Replaces 4 execution-apis revert/estimateGas fixtures that relied on fixed addresses with Sei fixtures that use a deployable reverter contract (REVERTER); script sets SEI_EVM_IO_REVERTER_ADDRESS.
Related: PLT-158
Testing performed to validate your change
Latest run: 162 tests, 133 passed, 29 failed (
82.1%pass) which previously was61.6%Failed cases are documented in FAILED_TEST_ANALYSIS.md with suggested RPC fixes.