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

Run only the tests which contains some mutation #50

Merged
merged 4 commits into from
Mar 12, 2018

Conversation

gnieto
Copy link
Collaborator

@gnieto gnieto commented Mar 8, 2018

Attempt to fix issue: #28

I want to get a little bit of feedback before keep going with that.

As a first step, all the mutations are being instrumented and are
calling ::mutagen::report_coverage(). This method, if an environment
variables is set (MUTAGEN_COVERAGE), the first time it will be called,
it will write to a specific file that some mutation has been hit.

On the other side, cargo-mutagen will check all the tests that the
binary contains, and will execute all the tests individually, with the
mentioned env var. After executing each of the tests, we will check if
the file has been created. If it exists, we know that the executed test
contains code that it's mutated.

Finally, when running mutations, we will run only the tests that
contains mutations (unfortunatelly, we will need to run the test
one-by-one, which means that we will spawn a process per test, instead
of a process per testsuite). Then, coverage flag it's only useful when
the ratio of tests which executes mutated code is low.

To test with coverage, after updating to the new version of the plugin
on the target project, and after reinstalling the runner:

cargo mutagen -- --coverage

Pending tasks would be (maybe new issues can be opened):

  • Improve the "ping" system with something better than a file
  • Maybe we can include the mutation number to report_coverage method,
    in order to know which are the mutations that took place on the given
    test. If we do, we can filter more accuretly which tests should be
    executed on each mutation
  • Maybe we can think about use clap to parse arguments, add subcommands,
    help, ...
  • Document the cargo mutagen command

@@ -187,6 +187,8 @@ impl<'a, 'cx> MutatorPlugin<'a, 'cx> {

mut_expression = quote_expr!(self.cx,
{
::mutagen::report_coverage();
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we add $n as an argument? Or even $n, $n + x (for some value of x if we have multiple mutations)? Because this way, we can not only detect whether mutations have been active at all, but also which mutations are covered, allowing us to restrict the set of tests to run per mutation even further.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added on: f9e543c

@llogiq
Copy link
Owner

llogiq commented Mar 9, 2018

Perhaps we should insert a static _mutation_$n: AtomicBool to restrict writing to the file to the first time around or something. clap is cool, but I'm personally a big fan of structopt. If the process per test is too costly, we may want to look into replacing the test runner with our own (sort of like stainless or whatever RFC #2318 will afford us).

Copy link
Owner

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

All in all this is great work!

src/coverage.rs Outdated
return
}

match COVERAGE_ALREDY_CHECKED.lock() {
Copy link
Owner

Choose a reason for hiding this comment

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

This is probably funny, but I find this to be a bit heavy. 😄

We could generate one static AtomicBool per mutation and use that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback, adressed on: 648be7c

I've needed an extra env var to be able to create a vector which is long enough. If there's any alternative, let me know.

@llogiq
Copy link
Owner

llogiq commented Mar 11, 2018

There are two other options:

  • on the first_block start, store the current mutation count and a generated symbol for the function. Then at the end of first_block, get the difference of the current count and the stored count and prepend a statement to the block with a static AtomicBool array of current_count - initial_count size. Then you can use this array from within the block (index at current_count - initial_count)
  • Generate one symbol per mutation, prepend the static AtomicBools at the end of first_block

Both options get rid of the allocation and the end var.

@gnieto
Copy link
Collaborator Author

gnieto commented Mar 11, 2018

@llogiq If I understand, you suggest to add static variables on the scope of the mutated code (so, added via first_block; which, as far as I can see, it's called with the body of any function/method/... that it's mutated).

Then, you suggest to add static local variables on that scope. But, does it makes sense? If i'm not wrong, the generated code will be something like:

pub fn test() -> usize {
    static COVERAGE_CHECKED: AtomicBool = AtomicBool::new(false);
    <original_code>
}

But, if test function is called more than once, it will create and drop the value every time is called, as far as i can see. Said with other words: does it make sense the static annotation if it's not added on the global scope? I think I'm missing something :/

Thanks!

@llogiq
Copy link
Owner

llogiq commented Mar 11, 2018

statics are directly embedded in the binary and are not initialized, see an example.

@llogiq
Copy link
Owner

llogiq commented Mar 11, 2018

(so in our extended case, we'd have a static $gensymed_arr : [AtomicBool ; $number_of_mutations] = [ATOMIC_BOOL_INIT; $number_of_mutations];)

Attempt to fix issue: llogiq#28

As a first step, all the mutations are being instrumented and are
calling `::mutagen::report_coverage()`. This method, if an environment
variables is set (`MUTAGEN_COVERAGE`), the first time it will be called,
it will write to a specific file that some mutation has been hit.

On the other side, `cargo-mutagen` will check all the tests that the
binary contains, and will execute all the tests individually, with the
mentioned env var. After executing any of the tests, we will check if
the file has been created. If it exists, we know that the executed test
contains code that it's mutated.

Finally, when running mutations, we will run only the tests that
contains mutations (unfortunatelly, we will need to run the test
one-by-one, which means that we will spawn a process per test, instead
of a process per testsuite). Then, coverage flag it's only useful when
the ratio of tests which executes mutated code is low.

To test with coverage, after updating to the new version of the plugin
on the target project, and after reinstalling the runner:

```
cargo mutagen -- --coverage
```

Pending tasks would be:
- Improve the "ping" system with something better than a filename
- Maybe we can include the mutation number to `report_coverage` method,
  in order to know which are the mutations that took place on the given
test. If we do, we can filter more accuretly which tests should be
executed on each mutation
- Maybe we can think about use clap to parse arguments, add subcommands,
  ...
Instead of track if a test has hit any mutation, now we are tracking
the exact mutations that has been hit.

Now we can execute ONLY the tests that are affected by a given mutation
identifier.
@gnieto
Copy link
Collaborator Author

gnieto commented Mar 11, 2018

Thanks again for the tip and the info of the local static vars!

I've been implementing it (you can check it here: 64faf7a), but I got the impression that code is getting more complicated and I'm not sure if the complexity is worth.
In fact, on the referenced commit, I'm getting errors when mutating code as quote_expr!(...) is evaluated before the new quote_stmts (the ones which define the new static variables) has been inserted on fold_first_block.

I was wondering if the code as it is now is enough or if it still needs refinement.

@llogiq
Copy link
Owner

llogiq commented Mar 12, 2018

I'm ok with merging now and refining later.

I'd just like to note that the reason I'm so worried about performance of this code is that it will be executed once for every call of the mutated method, and we don't know how often the tests will call it. So I don't worry much about the one-time cost (although I must confess that I've looked into using sockets instead of files), but the code that potentially gets run a lot should be fast.

To quell your errors, I think you could quote the static statement first (perhaps with a ! initializer), and insert the correct array initializer later.

@llogiq llogiq merged commit d3b000b into llogiq:master Mar 12, 2018
@llogiq
Copy link
Owner

llogiq commented Mar 12, 2018

I just merged so we can continue other development without too much hassle – I will try to optimize things later.

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.

None yet

2 participants