feat[test]: add more transient storage tests #3883
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3883 +/- ##
==========================================
- Coverage 90.89% 88.93% -1.96%
==========================================
Files 95 95
Lines 14388 14388
Branches 3186 3186
==========================================
- Hits 13078 12796 -282
- Misses 908 1130 +222
- Partials 402 462 +60 ☔ View full report in Codecov by Sentry. |
charles-cooper
changed the title
There was a problem hiding this comment.
this looks fine to me. to test that it's really transient, we could add additional tests that the variables are wiped between transactions.
also, we can improve on the test setup -- we don't need that branch in every single test. maybe we can add something to get_contract which expects failure for transient storage compilation exceptions?
tests/conftest.py
Outdated
| **kwargs, | ||
| ): | ||
| settings = Settings() | ||
| settings.optimize = override_opt_level or optimize | ||
|
|
||
| if has_transient_storage and not version_check(begin="cancun"): |
There was a problem hiding this comment.
instead of passing a parameter, let's either check the error message, or have a more specific exception type for transient storage
There was a problem hiding this comment.
how about this?
class UnsupportedTransientStorageException(EvmVersionException):
"""Transient storage is not supported for the active EVM ruleset."""
There was a problem hiding this comment.
sure, but let's do a bit shorter: TransientStorageException(EvmVersionException)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tests/conftest.py
Outdated
| if not version_check(begin="cancun"): | ||
| with pytest.raises(TransientStorageException): | ||
| compiler.compile_code(source_code) | ||
| return |
There was a problem hiding this comment.
we could even do pytest.skip so we can avoid those annoying version checks after get_contract()
There was a problem hiding this comment.
the alternative might be to use something like pytest.mark.transient, and then we can modify the test collector to skip those for pre-cancun
There was a problem hiding this comment.
the alternative might be to use something like pytest.mark.transient, and then we can modify the test collector to skip those for pre-cancun
does this mean we would not check for the same contracts that pre-cancun contracts using transient throws the correct exception?
There was a problem hiding this comment.
uhm, i think pytest.skip() raises some pytest internal exception. so it would check for the raise, and then skip.
tserg
changed the title
setup.cfg
Outdated
| @@ -34,6 +34,7 @@ python_files = test_*.py | |||
| testpaths = tests | |||
| markers = | |||
| fuzzing: Run Hypothesis fuzz test suite (deselect with '-m "not fuzzing"') | |||
| transient: Mark tests using transient storage for differentiation based on EVM version | |||
There was a problem hiding this comment.
this seems like it needs to be updated .. not sure why tests are not failing otherwise
There was a problem hiding this comment.
ah, we don't have --strict-markers enabled (see note here https://docs.pytest.org/en/latest/example/markers.html#registering-markers)
There was a problem hiding this comment.
do we want to enable --strict-markers?
There was a problem hiding this comment.
yes, probably, i have an open PR which touches setup.cfg though so we should wait until that is merged to avoid conflicts
| x: transient(uint256) | ||
|
|
||
| @external | ||
| @pure |
There was a problem hiding this comment.
this should go under pure tests tbh
There was a problem hiding this comment.
it would be good to have one for view as well
|
Maybe let's add some tests with stateful modules? |
|
Also, I don't see |
add complex transient storage tests
|
|
||
| c = get_contract(code) | ||
| assert c.get_my_list(*values) == list(values) | ||
| with pytest.raises(TransactionFailed): |
There was a problem hiding this comment.
is this on purpose instead of tx_failed()?
There was a problem hiding this comment.
nope, I was not aware of that! I will change to tx_failed().
charles-cooper
changed the title
this commit adds dedicated tests for basic operations in the transient storage location. it mostly follows the dedicated tests for immutables and copies them where relevant. it also adds a `requires_evm_version` marker, which can be used in future to test language features which should be blocked in certain EVM versions. --------- Co-authored-by: cyberthirst <[email protected]>
What I did
Follow tests for immutables and replicate where relevant.
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture