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

False positive uncovered mutant with switch(true) and switch(false) #819

Open
luispabon opened this issue Nov 8, 2019 · 10 comments
Open

Comments

@luispabon
Copy link
Contributor

luispabon commented Nov 8, 2019

Question Answer
Infection version 0.14.2
Test Framework version PHPUnit 8.4.3
PHP version 7.2.24/7.3.11/7.4.0-RC4
Platform Ubuntu/docker
Github Repo https://github.com/luispabon/favicon-finder
Example https://infection-php.dev/r/emew

Infection is reporting an uncovered mutant on the repository above:

Not Covered mutants:
====================


1) /home/luis/Projects/favicon/src/Favicon.php:155    [M] TrueValue

--- Original
+++ New
@@ @@
             $links = $dom->getElementsByTagName('link');
             foreach ($links as $link) {
                 /** @var DOMNode $link */
-                switch (true) {
+                switch (false) {
                     case $link->hasAttribute('rel') && strtolower($link->getAttribute('rel')) === 'shortcut icon':
                     case $link->hasAttribute('rel') && strtolower($link->getAttribute('rel')) === 'icon':
                     case $link->hasAttribute('href') && strpos($link->getAttribute('href'), 'favicon') !== false:

This is however a false positive. That line is indeed covered, and the switch from true to false does break tests.

You can check by cloning, composer -o install; make. This runs phpunit and infection in sequence, infection recycling test results from phpunit. Letting infection generate its own reports makes no difference.

Build: https://ci.auronconsulting.co.uk/teams/main/pipelines/favicon-finder-master/jobs/analyze-master/builds/1

@maks-rafalko
Copy link
Member

maks-rafalko commented Nov 8, 2019

Duplicate of #34

This is how coverage looks like:

Code Coverage for :home:vagrant:tmp:favicon-finder:src:Favicon php 2019-11-08 23-01-48

It boils down to how PHP works internally, please read exactly the same issue we created earlier #34

See Derick's explanation here https://bugs.xdebug.org/view.php?id=1479#c4450

I would say it won't be fixed on Infection side unless someone wants to do some magic to make it work (similar to how @BackEndTea did it for array coverage)

Closing for now, feel free to reopen, fix, and discuss farther though.

@maks-rafalko
Copy link
Member

maks-rafalko commented Nov 8, 2019

Probably, as a quick fix, it makes sense to not mutate switch(false) to switch(true) and vice-versa since we know it is always marked as Not Covered. What do you think @luispabon and @infection/core ?

@maks-rafalko maks-rafalko reopened this Nov 8, 2019
@BackEndTea
Copy link
Member

Is the switch never covered? Or only when its switching over true/a constant?

If its never covered we can always skip that; otherwise only for true and false mutator

@maks-rafalko
Copy link
Member

maks-rafalko commented Nov 8, 2019

Is the switch never covered?

I only saw not covered switch line for true/false. Just tried with an expression, and it is covered

Code Coverage 2


Let's do a quick fix to not mutate true -> false, false -> true in a switch. Should fix the issue for now. @luispabon feel free to help with the fix ;)

@luispabon
Copy link
Contributor Author

Happy to help, could you give me any pointers of where to look? Never got my hands dirty on infection's codebase

@BackEndTea
Copy link
Member

Probably somewhere around here:

protected function mutatesNode(Node $node): bool

My best bet would be a check to see if the direct parent is the switch statement.

@BackEndTea
Copy link
Member

For completeness sake this should also be for the false mutator

@sanmai
Copy link
Member

sanmai commented Nov 9, 2019

Harder, but better, will be to tag a switch as covered if any single line inside of it is covered.

@BackEndTea
Copy link
Member

I think we only need to check the case statements for that. So wed only need to go 1 level deep.

@sanmai
Copy link
Member

sanmai commented Nov 9, 2019

Or up, unconditionally tagging an uncovered switch as covered every time we see a covered case statement.

@maks-rafalko maks-rafalko changed the title False positive uncovered mutant False positive uncovered mutant with switch(true) and switch(false) Nov 13, 2019
@maks-rafalko maks-rafalko self-assigned this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants