Smaller-than-function mutations

[dmoka]

Feature request

It would be handy to have a mutation to remove statements that call a single function or method. I see many times in bigger projects (written without TDD) that such things are not covered by tests.

For example, let's say I have the following code:

impl EventBus {

    pub fn deposit_event(event_id: u32) {
        print!("event with id {} is emitted", event_id);
    }

    pub fn do_logic(event_id: u32) {
        //some logic
        Self::deposit_event(3);
    }
}

It would be nice to have a mutation of this code where we would delete the following line in the do_logic method:

    Self::deposit_event(3);

What do you think, it is feasible to do such a thing?

Note

If I run cargo mutants, the following mutant is generated

src/main.rs:12: replace EventBus::deposit_event with () ... NOT CAUGHT in 0.248s

It rules out many cases, but does not rule out the situation where we have tests for public API deposit_event, but not for calling it within other public API do_logic

Reproduction

Steps

1. Clone the following test repo
2. Navigate to subproject ./delete-statement
3. Execute cargo mutants

Expected behaviour:

There is a mutant generated for removing a statement calling a specific function, something like:

src/main.rs:15: remove Self::deposit_event(3) ... NOT CAUGHT in 0.248s

Actual behaviour:

No such mutation is supported by command cargo mutants

[sourcefrog]

Hey, thanks again for the idea.

I think there is interesting potential to generate smaller-than-function mutations. That's likely to greatly increase the number of mutations, which makes parallelization (#39) more important, and perhaps means people want to sample mutants rather than run them exhaustively. So, I would probably not tend to add this right yet, and maybe even then not turn it on by default.

So in your example, it seems to me that your problem is that you don't have very good tests for do_logic. (In fact, not to be rude 😉 but it doesn't have any meaningful tests at all.)

The mutation that you're proposing in this bug is in fact the exact same mutation that we already generate: hollowing out do_logic. No test pays attention to whether either deposit_event or do_logic did anything. cargo-mutants would already tell you this.

Assuming you wanted to keep the application logic the same, you could write a test that catches stdout and checks for the expected output, much like the CLI tests in cargo-mutants. That test could potentially catch changes to both these functions.

So I think a more actionable definition of this bug would be: here is a tree that is actually not very well tested, but cargo-mutants doesn't say so, but deleting individual statements would potentially catch it. I think one could write an example that does this and that doesn't look too contrived.

[sourcefrog]

In general this will help when the tests evaluate some, but not all, important behavior of the function. For example:

• The function returns a compound type such as a struct, and the tests check some but not all of the fields or observable behavior of the returned value. A mutation that replaces some of the values in the struct literal or in variables computed earlier in the function might catch this.
• The function returns a value, which is checked by the tests, and the value is nontrivial enough the the tests will catch straightforward replacement with e.g. Ok(()). However, the function has an important side effect, and the tests don't check it. Deleting the code that causes the side effect would demonstrate the gap in test coverage. (However, I think fairly often the side effect is itself caused by a function, and deleting the body of that function would also show the problem, as in the example above. But not always.)

[dmoka]

Great additions to the topic, I like them! Also agree with the point we should give the users the options to opt-in/opt-out specific mutants.

On the other hand, I would like to challenge my delete-statement idea further. Sorry, but my previous code example was not trivial to demonstrate the problem. The updated code can be found here. But let me first explain it:

Let's say we have some logic we want to use it in multiple parts of our program. In this case just a simple data Repository, storing some data:

pub struct Repository {
    pub data: Vec<String>
}

impl Repository {

    pub fn new() -> Self {
        Repository{
            data: Vec::<String>::new()
        }
    }

    pub fn store(&mut self, data_entry: &str) {
        self.data.push(data_entry.to_string());
    }

}

Now let's say we use this logic in two places in our program. For example in do_logic and do_logic2. These two functions do very similar things, they increment the input number with a given amount, and store some data in the repository. (Please ignore the cfg_attr for now)

impl Main {

    #[cfg_attr(test, mutate)]
    pub fn do_logic(repo: &mut Repository, mut number: u32) -> u32 {
        number = number + 5;

        repo.store("data from do_logic");

        number
    }

    #[cfg_attr(test, mutate)]
    pub fn do_logic2(repo: &mut Repository, mut number: u32) -> u32 {
        number = number + 10;

        repo.store("data from do_logic2");

        number
    }
}

Now let's say I totally cover my do_logic method with tests, resulting in 100% code coverage and mutation score. Next to that, I write a test for do_logic2 checking if the number is incremented, but I forgot to write a test for the data being stored in the repository. In this situation, I would expect the mutation framework to generate a surviving mutant telling that I do not cover the Repository store method in do_logic2 method in my test suite, namely this line.

For this code, the hollowing-out mutation generated by cargo mutants is caught, namely by a failing do_logic_should_store_data test. So If I run cargo mutants, then the result tells that all my mutants are killed/caught. But still there is a gaping hole in my test suite.

Next to that, there are situations where we have no control over the code we use (fe.: 3th party library), so we can not mutate it, so not possible to hollow it out.

I left in some cfg_attr relating to the mutagen library, you probably know this. Although this library is also cool, I am not a big fan of it as the user needs to pollute their production code with a lot of attributes to have the mutations generated, which is quite cumbersome in my opinion.

On the other hand, this mutagen library can generate a mutation which deletes the abovementioned method call. In the current workplace I work, we have multiple projects each containing dozens of packages. We found this delete-statement mutation as literally the most useful one, it helped us a lot in finding the gaping holes in our test suites. That's why I am pushing for this feature! x)

Let me know your thoughts! Thank you!

[sourcefrog]

Yeah, exactly. That's a more realistic example of the second category in my post.

I'm in favor of adding this, but I think it will need some mitigations for the likely effect on run time, e.g.

• an option for smaller than function mutants
• a run time limit
• parallel testing
• ...

[sourcefrog]

https://stryker-mutator.io/docs/mutation-testing-elements/supported-mutators/ has a good list of ideas.

[sourcefrog]

The upcoming 23.12 release can represent smaller-than-function mutants, and adds a few modifications of &&, ||, ==, !=, in #170 and #173. We can add more patterns.

Deleting single statements seems to have a lot of potential too, although it would make a lot of mutants, and I have some concern it might produce too much noise. Perhaps if we exclude patterns like trace lines it will be OK.

[kpreid]

Deleting single statements seems to have a lot of potential too, although it would make a lot of mutants

There could be categorical mutations like “delete all statements that aren't lets” (that is, delete all statements whose deletion doesn't cause the code not to compile due to an unbound variable). This avoids combinatoric explosion but is still likely to efficiently create interesting mutants by removing side-effects while keeping the return value.

(It might also be good to do this while keeping assert!(...); statements, since omitted assertions are potentially the sort of thing that's hard or tedious to test but has been thought about carefully anyway. Opinions may differ on that.)