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

refactor(tests): re-write test parametrization to avoid skipping invalid parameter sets #959

Open
danceratopz opened this issue Nov 20, 2024 · 3 comments
Labels
good first issue Good for newcomers scope:tests Scope: Test cases type:refactor Type: Refactor

Comments

@danceratopz
Copy link
Member

danceratopz commented Nov 20, 2024

Skipped tests when filling (for invalid parameter sets) unnecessarily pollute the console and are confusing for users.

Some examples can be found via:

uv run fill  tests/shanghai/eip3860_initcode/test_initcode.py --output=/tmp/fixtures -v -k test_gas_usage

The following test cases should never get created by creating a list of valid test cases via a helper test function:

tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-blockchain_test-too_little_execution_gas-empty] SKIPPED                               [ 19/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-blockchain_test-too_little_execution_gas-single_byte] SKIPPED                         [ 20/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-blockchain_test_engine-too_little_execution_gas-empty] SKIPPED                        [ 51/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-blockchain_test_engine-too_little_execution_gas-single_byte] SKIPPED                  [ 52/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-state_test-too_little_execution_gas-empty] SKIPPED                                    [ 83/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Shanghai-state_test-too_little_execution_gas-single_byte] SKIPPED                              [ 84/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-blockchain_test-too_little_execution_gas-empty] SKIPPED                                 [115/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-blockchain_test-too_little_execution_gas-single_byte] SKIPPED                           [116/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-blockchain_test_engine-too_little_execution_gas-empty] SKIPPED                          [147/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-blockchain_test_engine-too_little_execution_gas-single_byte] SKIPPED                    [148/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-state_test-too_little_execution_gas-empty] SKIPPED                                      [179/192] 
tests/shanghai/eip3860_initcode/test_initcode.py::TestContractCreationGasUsage::test_gas_usage[fork_Cancun-state_test-too_little_execution_gas-single_byte] SKIPPED                                [180/192]

I.e., these lines must be made unnecessary and removed:

pytest.skip(
"Special case, the execution of the initcode and gas "
"cost to deploy are zero: Then this test case is "
"equivalent to that of 'test_exact_intrinsic_gas'."
)

The resulting fixtures should be verified via uv run hasher with and without the refactor (all hashes bar the top-level folder should stay the same). For easy verification, it will be necessary that the existing test IDs from the valid cases are maintained and not rearranged, by careful ordering of the parameters.

The same technique can be applied to this test function as for ipsilon#5.

@danceratopz danceratopz added good first issue Good for newcomers scope:tests Scope: Test cases type:refactor Type: Refactor labels Nov 20, 2024
@MukulKolpe
Copy link
Contributor

Hey @danceratopz, can I work on this issue?

@danceratopz
Copy link
Member Author

Sure, go for it @MukulKolpe!

@0saurabh0
Copy link

@danceratopz, If we have to do multilple refactoring

I.e., these lines must be made unnecessary and removed:

  • what if we filter out invalid test cases during the test setup phase, ensuring only valid combinations are generated?
  • or maybe can Create a helper function to determine if a test case is valid and then use this function to filter out invalid test cases during parametrization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers scope:tests Scope: Test cases type:refactor Type: Refactor
Projects
None yet
Development

No branches or pull requests

3 participants