ExpressionRepeat mutator #1215
ExpressionRepeat mutator #1215
Conversation
| <?php | ||
|
|
||
| $z = $b->bar() + [1 => $c->foo()]; | ||
| $z = $b->bar() + [1 => $c->foo()]; |
There was a problem hiding this comment.
Won't we have many new hard-to-kill mutations with mutating any expression?
-
Probably it's safer to mutate only method calls with side effects? (see
MethodCallRemoval)for example, mutate this
$this->foo() +$this->foo()but not this
$a = $this->foo() + $this->bar(); + $a = $this->foo() + $this->bar();Or, probably, default to mutating only side-effects method calls but add a setting for this mutator to mutate any expression. Wdyt?
Now MSI is quite low for such mutator https://monosnap.com/direct/cb98GfVJArUligeK0FLRboh5dQrSa1
-
Also, what about static calls?
Any::foo()- is it mutated? let's add it to tests, as if it is, this will be extremely hard to kill
There was a problem hiding this comment.
Good point that is should try very hard to be as useful as it can. Let me see if I can limit it to only method-call containing statements.
There was a problem hiding this comment.
I updated it to mutate only statements with method calls, but without static or function calls. This way we can be certain a mutation can be tested against. Can you confirm on your end?
As I see it, we get:
343 mutations were generated:
157 mutants were killed
0 mutants were not covered by tests
186 covered mutants were not detected
0 errors were encountered
0 time outs were encountered
Metrics:
Mutation Score Indicator (MSI): 45%
Mutation Code Coverage: 100%
Covered Code MSI: 45%
I'd say we have far worse mutations, which are often almost impossible to test against.
| Mutator | Mutations | Killed | Escaped | Errors | Timed Out | MSI (%s) | Covered MSI (%s) |
|---|---|---|---|---|---|---|---|
| ExpressionRepeat | 343 | 162 | 181 | 0 | 0 | 47.23 | 47.23 |
| CastFloat | 7 | 3 | 4 | 0 | 0 | 42.86 | 42.86 |
| GreaterThanOrEqualTo | 7 | 3 | 4 | 0 | 0 | 42.86 | 42.86 |
| RoundingFamily | 14 | 6 | 8 | 0 | 0 | 42.86 | 42.86 |
| UnwrapArrayValues | 5 | 2 | 3 | 0 | 0 | 40.00 | 40.00 |
| UnwrapStrToLower | 1 | 0 | 1 | 0 | 0 | 0.00 | 0.00 |
| ProtectedVisibility | 1 | 0 | 1 | 0 | 0 | 0.00 | 0.00 |
| LessThanOrEqualTo | 1 | 0 | 1 | 0 | 0 | 0.00 | 0.00 |
| CastObject | 1 | 0 | 1 | 0 | 0 | 0.00 | 0.00 |
|
This mutation will show insufficient coverage in cases where one does not verify that a certain method called only once. Killing is a simple as adding |
|
I must say, I'm really not convinced :/
There is 3 things bothering me in this argument:
You are effectively adding code, and asking the user to test that this code should not be able to be added. To me it is in the same line as creating a dummy interface, adding an interface to a class and asking the users to kill that mutation. And sure you can, it's as simple as adding Now I'm aware I am biased against semantic additions, so let's get another axe of discussion as well. I'm trying your change on PHP-Scoper, here's the difference: masterExpressionRepeatSo the result is:
But those are just indicators we added mutations, it itself it just shows the cost of that new mutator but not the benefits. So I took the extra effort and time to look at the report. And I genuinely looked for cases I might be interested in which I feel my tests where lacking, but I could not find any. So there is two possibilities:
$this->eol = chr(10);
+ $this->eol = chr(10);// This method is without side-effects
$whitelistedFunctions = $this->whitelist->getRecordedWhitelistedFunctions();
+ $whitelistedFunctions = $this->whitelist->getRecordedWhitelistedFunctions();// This method is without side-effects
$hasNamespacedFunctions = $this->hasNamespacedFunctions($whitelistedFunctions);
+ $hasNamespacedFunctions = $this->hasNamespacedFunctions($whitelistedFunctions);// This method is not without side-effect, but is idempotent
// this might lead to a slight performance impact, but nothing worth me carrying about
$dump = $this->cleanAutoload($dump);
+ $dump = $this->cleanAutoload($dump); if ($hasNamespacedFunctions) {
$eol = $this->eol;
+ $eol = $this->eol; $original = new FullyQualified($node[0]);
+ $original = new FullyQualified($node[0]); self::validateConfigKeys($config);
+ self::validateConfigKeys($config);
I'm more than willing to carry on on the debate with an open mind and I'm sure you can find some ways to improve the mutator. In its current form however, this mutator is a red flag to me. |
sanmai
force-pushed
the
pr/2020-03/ExpressionRepeat
branch
from
95ddae9
to
0f10ccf
Compare
Testing defects, just like any other mutation. That's mutation testing is for, am I not right? For example, this is a legitimate defect: $a = $this->dependency->veryLongExpensiveOperation();
+$a = $this->dependency->veryLongExpensiveOperation();If you're not testing against these defects, Infection will point this out. It will also remind you with:
|
|
I think it should be done the other way around: to ask to justify the existing code rather than justify the potentially added code. So with your example, let's say you have: $a = $this->dependency->veryLongExpensiveOperation();
$a = $this->dependency->veryLongExpensiveOperation();I would rather have the process of: $a = $this->dependency->veryLongExpensiveOperation();
-$a = $this->dependency->veryLongExpensiveOperation();and then justify that you need it called twice and not once. And eventually even: -$a = $this->dependency->veryLongExpensiveOperation();(provided we can do so without creating too many false-positives) |
|
IIRC we have this the-other-way-around mutation, and it is awful TBH. For example, not only you won't be testing against these mutations, in many cases you just can't: - Assert::notNull($example);They're true false positives. I think it can also make use of this new |
It's the same with this mutator though: 64% of the generated ones are escaped in PHP-Scoper and 0% of it was something I really wanted to check against (regardless of its feasibility). So the removal process makes a lot more sense to me, but whether we can pull it off in an efficient way (i.e. not too many false positives) is a different story. |
|
I'm not sure to understand |
|
We can tell that a call most likely without testable side-effects if it is a static call or function call. Therefore we do not mutate them here. Yeah, it is easy to make this mutator alter statements with static calls, but who we are to punish people using a bad API without DI? I don't think this is where we should stand. |
|
I'm not sure I like this mutator. I get that it can be very useful, but I also think that many mutations will be very hard to kill. Consider the following: final class Foo {
private ?string $bar = null;
public function setBar(string $bar): void {
$this->bar = $bar;
}
// ...
}
// Somewhere else:
$foo->setBar('test');How am I supposed to kill the duplication of What I like about all of the existing mutators is that they don't produce any false positives. At least I haven't found any so far. Everything Infection tells me about is a legitimate problem that I can address. This mutator would introduce false positives I can't do anything about. Maybe it could be left out of the default mutators and users would have to manually enable it or something..? |
sanmai
force-pushed
the
pr/2020-03/ExpressionRepeat
branch
from
1f775fd
to
81b26df
Compare
This PR:
ExpressionRepeatmutator, within a new family ofAugmentationThis mutator duplicates statements with dynamic calls:
$this->foo(); +$this->foo();But leaves static and plain function calls alone, even if they're mixed together with a dynamic call. Therefore we can say these new mutations are 100% testable, although it cat be easier to let them be sometimes.
This mutator will show insufficient coverage in cases where one does not verify that a certain method called only once, or exact number of times.
Killing these mutations is a simple as adding
->expects($this->once())to a mock.Design drawbacks
Current implementation for the
ImpureExpressionVisitorgood enough to find impure statements, but may not find statements, that include other impure statements, as impure. This means the mutator creates less mutations than it possibly can otherwise, but at this point it may be sensible to keep the number low.