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

feat(fw): use pytest nodeids as test names in json fixtures #329

Closed
danceratopz opened this issue Oct 11, 2023 · 3 comments · Fixed by #342
Closed

feat(fw): use pytest nodeids as test names in json fixtures #329

danceratopz opened this issue Oct 11, 2023 · 3 comments · Fixed by #342
Labels
scope:fw Scope: Framework (evm|tools|forks|pytest) scope:pytest Scope: Changes EEST's pytest plugins type:feat type: Feature

Comments

@danceratopz
Copy link
Member

danceratopz commented Oct 11, 2023

More info in this discussion: #307 (comment)

This will also mean removing the tag keyword arguments from tests, which is currently appended to the test names. I think we should add tags though, but not as a name, rather as metadata to allow filtering of tests, by opcode for example, something like tags=("create", "tstore", "tload", "reentrancy").

@danceratopz danceratopz added scope:pytest Scope: Changes EEST's pytest plugins type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Oct 11, 2023
@danceratopz
Copy link
Member Author

Additional observation that supports this change:

As of v1.0.6, the test fixture names (the keys in JSON fixture files) only consist of test parameters (and don't include the test function name) which leads to duplicate test IDs (as seen from a client test runner).

For example, these tests:

fill --evm-bin=~/code/github/danceratopz/go-ethereum/build/bin/evm --until=Cancun -k no_outer_selfdestruct --collect-only -q

tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_created_in_same_tx_with_revert[fork=Cancun-no_outer_selfdestruct]
tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_not_created_in_same_tx_with_revert[fork=Cancun-no_outer_selfdestruct]

produce the following fixtures with identical names:

ack no_outer_selfdestruct fixtures

fixtures/cancun/eip6780_selfdestruct/selfdestruct_revert/selfdestruct_created_in_same_tx_with_revert.json
2:    "000-fork=Cancun-no_outer_selfdestruct": {

fixtures/cancun/eip6780_selfdestruct/selfdestruct_revert/selfdestruct_not_created_in_same_tx_with_revert.json
2:    "000-fork=Cancun-no_outer_selfdestruct": {

@danceratopz
Copy link
Member Author

Another consideration for the rename is that dotnet fails with "=" when filtering via test Name. Although, the doc claims you can escape "=", I couldn't get either of the following to work:

dotnet test src/Nethermind/Ethereum.Blockchain.Pyspec.Test/ -c debug --no-build --no-restore --filter Name~"000-tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_created_in_same_tx_with_revert[fork=Cancun-no_outer_selfdestruct]"

dotnet test src/Nethermind/Ethereum.Blockchain.Pyspec.Test/ -c debug --no-build --no-restore --filter Name~"000-tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_created_in_same_tx_with_revert[fork\=Cancun-no_outer_selfdestruct]"

This hack using the AND operator works:

dotnet test src/Nethermind/Ethereum.Blockchain.Pyspec.Test/ -c debug --no-build --no-restore --filter Name~"000-tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_created_in_same_tx_with_revert[fork&Cancun-no_outer_selfdestruct]"
# -->
Test run for /home/dtopz/nethermind/src/Nethermind/Ethereum.Blockchain.Pyspec.Test/bin/debug/net7.0/Ethereum.Blockchain.Pyspec.Test.dll (.NETCoreApp,Version=v7.0)
Microsoft (R) Test Execution Command Line Tool Version 17.4.1+79d56b02b69a582cd90428878a5e9411ab7538f5 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
Running 000-tests/cancun/eip6780_selfdestruct/test_selfdestruct_revert.py::test_selfdestruct_created_in_same_tx_with_revert[fork=Cancun-no_outer_selfdestruct], Network: [Cancun] at 14:15:34.111680


Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: 549 ms - Ethereum.Blockchain.Pyspec.Test.dll (net7.0)

@danceratopz
Copy link
Member Author

danceratopz commented Nov 2, 2023

The pytest -k flag always had problems with "=", too:

fill --until=Cancun -k 'test_withdrawing_to_precompiles[fork=Cancun'

results in:

ERROR: Wrong expression passed to '-k': test_withdrawing_to_precompiles[fork=Cancun: at column 38: unexpected character "="

It doesn't seem possible to esacpe "=" in a -k expression either.

I'd suggest replacing "=" with "_" in our generated parameter strings used in nodeids in L702 here:

def pytest_make_parametrize_id(config, val, argname):
"""
Pytest hook called when generating test ids. We use this to generate
more readable test ids for the generated tests.
"""
return f"{argname}={val}"

https://github.com/ethereum/execution-spec-tests/blob/main/src/pytest_plugins/test_filler/test_filler.py#L697-L702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fw Scope: Framework (evm|tools|forks|pytest) scope:pytest Scope: Changes EEST's pytest plugins type:feat type: Feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant