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

WIP: basic mutation testing with cargo mutants #2232

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sveitser
Copy link
Collaborator

@sveitser sveitser commented Oct 29, 2024

Close #2227

  • Update nixpkgs to get latest cargo-mutants.
  • Remove nixWithFlakes from flake.nix because it would have to be changed to work with the latest version of nixpkgs and I think we no longer need it.
  • Create a non-flaky test profile.
  • Run with cargo mutants.
  • Inspect output and ./mutants.out directory

For example ./mutants.out/missed.txt looks like this

contract-bindings/src/plonk_verifier_2.rs:463:13: replace plonk_verifier_2::<impl AbiEncode for PlonkVerifier2Calls>::encode -> Vec<u8> with vec![]
hotshot-state-prover/src/mock_ledger.rs:496:57: replace + with * in gen_circuit_for_test
hotshot-state-prover/src/mock_ledger.rs:496:53: replace * with / in gen_circuit_for_test
hotshot-state-prover/src/mock_ledger.rs:149:9: replace MockLedger::elapse_without_block with ()
contract-bindings/src/light_client_mock.rs:1928:21: replace == with != in light_client_mock::<impl ContractRevert for LightClientMockErrors>::valid_selector
contract-bindings/src/erc1967_proxy.rs:319:13: replace erc1967_proxy::<impl AbiEncode for ERC1967ProxyErrors>::encode -> ::std::vec::Vec<u8> with vec![0]
hotshot-state-prover/src/circuit.rs:236:29: replace % with / in build

To make it really useful further configuration is likely necessary, such as marking some functions to not mutate them.

- Update nixpkgs to get latest cargo-mutants.
- Remove nixWithFlakes from flake.nix because it would have to be
  changed to work with the latest version of nixpkgs and I think we no
  longer need it.
- Create a non-flaky test profile.
- Run with `cargo mutants`.
- Inspect output and `./mutants.out` directory

For example `./mutants.out/missed.txt` looks like this

    contract-bindings/src/plonk_verifier_2.rs:463:13: replace plonk_verifier_2::<impl AbiEncode for PlonkVerifier2Calls>::encode -> Vec<u8> with vec![]
    hotshot-state-prover/src/mock_ledger.rs:496:57: replace + with * in gen_circuit_for_test
    hotshot-state-prover/src/mock_ledger.rs:496:53: replace * with / in gen_circuit_for_test
    hotshot-state-prover/src/mock_ledger.rs:149:9: replace MockLedger::elapse_without_block with ()
    contract-bindings/src/light_client_mock.rs:1928:21: replace == with != in light_client_mock::<impl ContractRevert for LightClientMockErrors>::valid_selector
    contract-bindings/src/erc1967_proxy.rs:319:13: replace erc1967_proxy::<impl AbiEncode for ERC1967ProxyErrors>::encode -> ::std::vec::Vec<u8> with vec![0]
    hotshot-state-prover/src/circuit.rs:236:29: replace % with / in build

To make it really useful further configuration is likely necessary, such
as marking some functions to not mutate them.
@sveitser sveitser mentioned this pull request Oct 29, 2024
@@ -156,6 +156,16 @@ strip = "debuginfo"
[profile.dev.package."*"]
opt-level = 3

[profile.test]
inherits = "dev"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test already inherits dev

@tbro
Copy link
Contributor

tbro commented Oct 29, 2024

Can you comment about how we are meant to read the output? What does this mean:

contract-bindings/src/plonk_verifier_2.rs:463:13: replace plonk_verifier_2::<impl AbiEncode for PlonkVerifier2Calls>::encode -> Vec<u8> with vec![]

cargo mutants made that replacement? What was the outcome? What was the expectation?

@@ -1,7 +1,15 @@
# cargo-mutants does not seem to support specifying the nextest profile? :(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the env var NEXTEST_PROFILE

@sveitser
Copy link
Collaborator Author

sveitser commented Oct 30, 2024

cargo mutants made that replacement? What was the outcome? What was the expectation?

@tbro this is still WIP and I will document it better once I think it's useful and I understand it well enough.

cargo mutatants makes copies of the codebase, makes some modifications, compiles the code and runs the tests.

The expectation is that for every change that cargo mutants makes there is at least one test that fails as a result. If not it's considered a mutant that our tests "missed".

However to make it effective we need to annotate some code so that cargo mutants doesn't mutate it (for example it's likely not useful if some change in mock_ledger is "missed"). There is also a bug I need to sort out where I get a warning from cargo mutants that the test run exited with an unexpected status code. I think it currently leads to false positive "missed" results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mutation testing to identify accidentally untested code
2 participants