search

ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
865 stars 287 forks source link

Fix exported error messages for invalid tx tests #886

Closed pdobacz closed 6 months ago

pdobacz commented 7 months ago

Addresses feedback from previous PR: https://github.com/ethereum/evmone/pull/858#discussion_r1572817857

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 37.50000% with 30 lines in your changes are missing coverage. Please review.

Please upload report for BASE (master@633f696). Learn more about missing BASE report.

:exclamation: Current head 4589219 differs from pull request most recent head 1516682

Please upload reports for the commit 1516682 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #886 +/- ## ========================================= Coverage ? 98.25% ========================================= Files ? 130 Lines ? 15672 Branches ? 0 ========================================= Hits ? 15398 Misses ? 274 Partials ? 0 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/886/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [ethereum-tests](https://app.codecov.io/gh/ethereum/evmone/pull/886/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `27.90% <0.00%> (?)` | | | [ethereum-tests-silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/886/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.66% <0.00%> (?)` | | | [execution-spec-tests](https://app.codecov.io/gh/ethereum/evmone/pull/886/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.00% <0.00%> (?)` | | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/886/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `94.11% <37.50%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ethereum/evmone/pull/886?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [test/state/state.hpp](https://app.codecov.io/gh/ethereum/evmone/pull/886?src=pr&el=tree&filepath=test%2Fstate%2Fstate.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS9zdGF0ZS5ocHA=) | `100.00% <ø> (ø)` | | | [test/unittests/state\_transition.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/886?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbi5jcHA=) | `98.40% <100.00%> (ø)` | | | [test/state/state.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/886?src=pr&el=tree&filepath=test%2Fstate%2Fstate.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS9zdGF0ZS5jcHA=) | `91.45% <34.78%> (ø)` | |
pdobacz commented 7 months ago

The missing messages PR'd to retesteth in https://github.com/ethereum/retesteth/pull/226

pdobacz commented 7 months ago

Codecov is complaining about new lines not being covered, but from what I understand, covering them would require adding new state_transition_tx.test to cover invalid blob tx (among others). Should we be doing that?

Alternatively we can remove the never used entries from the get_tests_invalid_tx_message switch, leaving it to fail on assert(false). Maybe actually this is better, at the expense of tidiness of the switch. WDYT @gumb0 ?