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

List of source directories is not respected in regard of more extensive coverage report #1264

Closed
ndench opened this issue Apr 27, 2020 · 10 comments · Fixed by #1269
Closed

Comments

@ndench
Copy link

ndench commented Apr 27, 2020

Question Answer
Infection version 0.16.3
Test Framework version PHPUnit 8.5.4
PHP version 7.4.5
Platform e.g. Ubuntu 16.04
Github Repo -

In order to run infection with 100% msi, we break our business logic into a "functional core" which can be thoroughly unit tested and kill every mutant. Then we run infection only over the functional core by using the "--testsuite=unit" test framework option and by using source.directories in infection.json.dist. With version 0.15.* this works exactly as expected, with mutants only being applied to our functional core:

  • 178 files mutated
  • 940 mutants generated

With version 0.16.3, infection is mutating our entire codebase:

  • 1578 files mutated
  • 10211 mutants generated
phpunit.xml
<?xml version="1.0" encoding="UTF-8"?>

<!-- https://phpunit.de/manual/current/en/appendixes.configuration.html -->
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/8.3/phpunit.xsd"
        backupGlobals="false"
        colors="true"
        bootstrap="config/bootstrap.php"
>
   <php>
       <ini name="error_reporting" value="-1" />
       <server name="APP_ENV" value="test" force="true" />
       <server name="SYMFONY_PHPUNIT_VERSION" value="8.3.5" />
       <server name="SHELL_VERBOSITY" value="-1" />
   </php>

   <testsuites>
       <testsuite name="All">
           <directory>tests/</directory>
       </testsuite>

       <!-- Different test types you might run separately -->
       <testsuite name="Unit">
           <directory>tests/Unit/</directory>
       </testsuite>
       <testsuite name="Integration">
           <directory>tests/Integration/</directory>
       </testsuite>
       <testsuite name="Message">
           <directory>tests/Message/</directory>
       </testsuite>
       <testsuite name="Contract">
           <directory>tests/Contract/</directory>
       </testsuite>
       <testsuite name="Model">
           <directory>tests/Model</directory>
       </testsuite>
       <testsuite name="Task">
           <directory>tests/Task</directory>
       </testsuite>

       <testsuite name="Functional">
           <directory>tests/Functional/</directory>
       </testsuite>

       <testsuite name="FunctionalAdmin">
           <directory>tests/Functional/Admin</directory>
       </testsuite>
       <testsuite name="FunctionalExport">
           <directory>tests/Functional/Export</directory>
       </testsuite>
       <testsuite name="FunctionalPlaybook">
           <directory>tests/Functional/Playbook</directory>
       </testsuite>
       <testsuite name="FunctionalUser">
           <directory>tests/Functional/User</directory>
       </testsuite>
       <testsuite name="FunctionalNegotiate">
           <directory>tests/Functional/Negotiate</directory>
       </testsuite>
       <testsuite name="FunctionalActivityStream">
           <directory>tests/Functional/ActivityStream</directory>
       </testsuite>
   </testsuites>

   <filter>
       <whitelist>
           <directory>./src/</directory>
       </whitelist>
   </filter>

   <listeners>
       <listener class="Hyra\Tests\Resources\TestListener\TestTidierListener" />
   </listeners>
</phpunit>
infection.json.dist
{
 "timeout": 30,
 "source": {
   "directories": [
     "src/Security",
     "src/*/Core",
     "src/*/Action",
     "src/Integrations/*/Action",
     "src/Algorithms/Derrida/Butler/Core"
   ],
   "excludes": [
     "Migrations"
   ]
 },
 "testFrameworkOptions": "--testsuite=Unit",
 "mutators": {
   "@default": true,
   "IdenticalEqual" : false,
   "NotIdenticalNotEqual": false,
   "MBString": false
 },
 "logs": {
   "text": "build\/infection\/log.txt",
   "debug": "build\/infection\/debug.txt",
   "summary": "build\/infection\/summary.txt"
 }
}

I tried stepping through with xdebug to track down the issue but got lost as so much has changed from 0.15 to 0.16. Happy to keep debugging if someone can help point me in the right direction?

@sanmai
Copy link
Member

sanmai commented Apr 28, 2020

That's a BC break in this version: it always mutates "covered" files unless you exclude them with a filter or from coverage altogether.

The easiest way to do the latter is to configure phpunit.xml in include only the files you want to be mutated in the whitelist.

   <filter>
       <whitelist>
           <directory>./src/</directory>
       </whitelist>
   </filter>

There's also a blacklist of sorts if that's more convenient.

Just for the completeness of the report, would you mind showing your infection.json.dist?

@ndench
Copy link
Author

ndench commented Apr 28, 2020

@sanmai thanks for the information, I've updated the issue to include my infection.json.dist.

Just to clarify, are you saying we filter the coverage generation to only include the "functional core" and anything code that's not covered by unit tests will then not be mutated?

Is this an expected BC break? It's not specified in the changelog or the blog post.

@sanmai
Copy link
Member

sanmai commented Apr 28, 2020

Unfortunately, it isn't explicitly specified. Nor thought over much (by me), for that matter.

Yes, it should work that if you filter the coverage to include only things you want to be mutated, then only included files will be mutated. I use this feature every time when I need to iterate quickly over a piece of code where I couldn't care about all other code.

You have to keep your configuration as it is, though.

@maks-rafalko
Copy link
Member

maks-rafalko commented Apr 28, 2020

I would say this is a bug, not an expected BC break between 0.15 and 0.16.

From our docs:

source.directories - array, contains all folders with source code you want to mutate. Can be ., but make sure to exclude vendor in this case.

So, the original purpose of this config setting is to limit all the folders inside the project to those that really need to be mutated. Now this is not the case.

Please see here, I've pushed a test that works for 0.15 but fails for 0.16.

  • 0.15: bin/infection - works
  • 0.15: bin/infection --only-covered - works
  • 0.16: bin/infection - does not work <------ here is a bug
  • 0.16: bin/infection --only-covered - works

We already have a similar bug and fixed it for 0.16.2 - #1238 - but that one was about source.excludes folders, not source.directories.

Explanation of the issue:

In 0.16, we have 2 sources of files to look at:

  1. from SourceFileCollector - this is an iterable that respects both source.directories and source.excludes
  2. from coverage report (PhpUnitXmlCoverageTraceProvider) - this is an iterable built from code coverage report. Before 0.16.2 it didn't respect source.excludes, now it does. But at the same time now it does not respect source.directories, and this is exactly the reason of the issue that @ndench is experiencing.

So, given the following project structure:

src
├── ToMutate
│   └── SourceClass.php
└── ToNotMutate
    └── ToNotMutateSourceClass.php

and the following infection.json config:

{
    "source": {
        "directories": [
            "src/ToMutate"
        ]
    }
}

we should not mutate src/NotToMutate folder as it was in 0.15.

Hot to fix?

I didn't have enough time to dig into the code yet, but I would say that the fix will be similar to source.excludes one's in #1238 - FilteredEnrichedTraceProvider::provideTraces() should return filtered out collection respecting infection.json config.

@sanmai
Copy link
Member

sanmai commented Apr 28, 2020

I didn't have enough time to dig into the code yet, but I would say that the fix will be similar to source.excludes one's in #1238 - FilteredEnrichedTraceProvider::provideTraces() should return filtered out collection respecting infection.json config.

Otherwise put, we have to find every file inside those source directories, and check against this list for every file we get from the coverage report. If a covered file isn't there, then we should skip it. True it is annoying, but not impossible.

Files in source directories will act as a whitelist in this scheme.

@maks-rafalko
Copy link
Member

Otherwise put, we have to find every file inside those source directories, and check against this list for every file we get from the coverage report. If a covered file isn't there, then we should skip it.

I see it exactly the same :)

@sanmai sanmai changed the title [v0.16] mutating undesired code List of source directories is not respected in regard of more extensive coverage report May 1, 2020
@theofidry
Copy link
Member

I think another usage in the wild is https://github.com/nelmio/alice, it currently does not work in 0.16 due to this (if you want to try it run make tm)

@sanmai
Copy link
Member

sanmai commented May 15, 2020

Excludes should have been working, are they not?

If not, that's a slightly different issue.

@theofidry
Copy link
Member

theofidry commented May 15, 2020 via email

@helpr helpr bot added the Has PR label Jun 6, 2020
sanmai added a commit that referenced this issue Jun 25, 2020
* Add test that passes for 0.15 but fails for 0.16 

* Fixes #1264

* Use source files as a whitelist inside FilteredEnrichedTraceProvider

* Move filtering OP earlier in the process

* Fix autreview warnings in FilteredEnrichedTraceProvider

* Simplify FilteredEnrichedTraceProvider

* UncoveredTraceProvider

* Refactor FilteredEnrichedTraceProvider into BufferedSourceFileFilter + UncoveredTraceProvider + UnionTraceProvider

* Improve BufferedSourceFileFilter, fix PHPStan warnings

* Factor out CoveredTraceProvider from UnionTraceProvider

* UncoveredTraceProviderTest

* CoveredTraceProviderTest

* BufferedSourceFileFilterTest

* UnionTraceProviderTest, removed FilteredEnrichedTraceProviderTest

* Fix PHPStan warnings in tests

* Fix remaining usages of FilteredEnrichedTraceProvider

Co-authored-by: borNfreee <b0rn@list.ru>
@helpr helpr bot added PR merged and removed Has PR labels Jun 25, 2020
@maks-rafalko maks-rafalko added this to the 0.17.0 milestone Jun 25, 2020
sanmai added a commit to sanmai/infection that referenced this issue Jun 25, 2020
Backport of infection#1269

* Fixes infection#1264

Co-authored-by: borNfreee <b0rn@list.ru>
@helpr helpr bot added the Has PR label Jun 25, 2020
sanmai added a commit that referenced this issue Jun 25, 2020
* Respect list of source directories

Backport of #1269

* Fixes #1264

Co-authored-by: borNfreee <b0rn@list.ru>

* CS

Co-authored-by: borNfreee <b0rn@list.ru>
@helpr helpr bot removed the Has PR label Jun 25, 2020
@maks-rafalko
Copy link
Member

Released https://github.com/infection/infection/releases/tag/0.16.4

Thank you for reporting and waiting for the fix ;)

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

Successfully merging a pull request may close this issue.

4 participants