Skip to content

Commit

Permalink
[Performance] Add files to coverage whitelist instead of the whole di…
Browse files Browse the repository at this point in the history
…rectories when `--filter` or `--git-diff-filter` are used (#1543)

* Performance improvement: add files to coverage whitelist instead of the whole directories when `--filter` or `--git-diff-filter` are used

Currently, when we build the initial tests `phpunit.xml` config, we add (if doesn't exist) the following coverage whitelist:

```xml
<phpunit>
    <coverage>
        <include>
            <directory>src/</directory>
            <directory>example/</directory>
        </include>
    </coverage>
</phpunit>
```

It means that **all** the files from directories `src/` and `example/` will be processed by coverage driver (be it `xdebug`, `pcov` or `phpdbg`
).

Collecting code coverage costs a lot, that's why this change tries to reduce the number of files added to coverage whitelist, replacing `<directory>` tag with N `<file>` tags if possible.

And one of the case is when we use `--filter=src/path/to/File1.php,src/path/to/File2.php` option, or when we use `--git-diff-filter=AM` option, that results internally to `--filter=X,Y,Z`.

When the filter is used, we 100% know that we will mutate only particular files, so we need the code coverage only for them.

Result initial `phpunit.xml` file will be like this:

```
infection --filter=src/path/to/File1.php,src/path/to/File2.php
```

```xml
<phpunit>
    <coverage>
        <include>
            <file>src/path/to/File1.php/</file>
            <file>src/path/to/File2.php/</file>
        </include>
    </coverage>
</phpunit>
```

This will dramatically decrease the time needed for collecting coverage data since we reduce the number of processed files.

* Use not-nullable array type for filtered source files to mutate

* Use empty array instead of `null` as a default argument
  • Loading branch information
maks-rafalko committed Aug 5, 2021
1 parent cfc3263 commit 193ab76
Show file tree
Hide file tree
Showing 13 changed files with 250 additions and 44 deletions.
10 changes: 5 additions & 5 deletions devTools/phpstan-src-baseline.neon
Expand Up @@ -305,6 +305,11 @@ parameters:
count: 1
path: ../src/TestFramework/Coverage/XmlReport/XPathFactory.php

-
message: "#^Parameter \\#1 \\$it of function iterator_to_array expects Traversable, iterable\\<Infection\\\\TestFramework\\\\Coverage\\\\Trace\\|SplFileInfo\\> given\\.$#"
count: 1
path: ../src/TestFramework/Factory.php

-
message: "#^Method Infection\\\\TestFramework\\\\PhpUnit\\\\Adapter\\\\PestAdapterFactory\\:\\:create\\(\\) has parameter \\$sourceDirectories with no value type specified in iterable type array\\.$#"
count: 1
Expand Down Expand Up @@ -360,11 +365,6 @@ parameters:
count: 1
path: ../src/TestFramework/PhpUnit/Config/XmlConfigurationManipulator.php

-
message: "#^Only booleans are allowed in an if condition, int given\\.$#"
count: 2
path: ../src/TestFramework/PhpUnit/Config/XmlConfigurationManipulator.php

-
message: "#^Only booleans are allowed in an if condition, int given\\.$#"
count: 6
Expand Down
1 change: 1 addition & 0 deletions src/Container.php
Expand Up @@ -290,6 +290,7 @@ public static function create(): self
$container->getTestFrameworkFinder(),
$container->getDefaultJUnitFilePath(),
$config,
$container->getSourceFileFilter(),
GeneratedExtensionsConfig::EXTENSIONS
);
},
Expand Down
5 changes: 2 additions & 3 deletions src/FileSystem/SourceFileCollector.php
Expand Up @@ -56,7 +56,7 @@ public function collectFiles(
array $excludeDirectories
): iterable {
if ($sourceDirectories === []) {
return;
return [];
}

$finder = Finder::create()
Expand All @@ -70,7 +70,6 @@ public function collectFiles(
$finder->notPath($excludeDirectory);
}

// Generator here to make sure these files used only once
yield from $finder;
return $finder;
}
}
38 changes: 36 additions & 2 deletions src/TestFramework/Factory.php
Expand Up @@ -35,17 +35,22 @@

namespace Infection\TestFramework;

use function array_filter;
use function array_map;
use function implode;
use Infection\AbstractTestFramework\TestFrameworkAdapter;
use Infection\AbstractTestFramework\TestFrameworkAdapterFactory;
use Infection\Configuration\Configuration;
use Infection\FileSystem\Finder\TestFrameworkFinder;
use Infection\FileSystem\SourceFileFilter;
use Infection\TestFramework\Config\TestFrameworkConfigLocatorInterface;
use Infection\TestFramework\PhpUnit\Adapter\PestAdapterFactory;
use Infection\TestFramework\PhpUnit\Adapter\PhpUnitAdapterFactory;
use InvalidArgumentException;
use function is_a;
use function iterator_to_array;
use function Safe\sprintf;
use SplFileInfo;
use Webmozart\Assert\Assert;

/**
Expand All @@ -59,6 +64,7 @@ final class Factory
private TestFrameworkFinder $testFrameworkFinder;
private string $jUnitFilePath;
private Configuration $infectionConfig;
private SourceFileFilter $sourceFileFilter;

/**
* @var array<string, array<string, mixed>>
Expand All @@ -75,6 +81,7 @@ public function __construct(
TestFrameworkFinder $testFrameworkFinder,
string $jUnitFilePath,
Configuration $infectionConfig,
SourceFileFilter $sourceFileFilter,
array $installedExtensions
) {
$this->tmpDir = $tmpDir;
Expand All @@ -83,11 +90,14 @@ public function __construct(
$this->jUnitFilePath = $jUnitFilePath;
$this->infectionConfig = $infectionConfig;
$this->testFrameworkFinder = $testFrameworkFinder;
$this->sourceFileFilter = $sourceFileFilter;
$this->installedExtensions = $installedExtensions;
}

public function create(string $adapterName, bool $skipCoverage): TestFrameworkAdapter
{
$filteredSourceFilesToMutate = $this->getFilteredSourceFilesToMutate();

if ($adapterName === TestFrameworkTypes::PHPUNIT) {
$phpUnitConfigPath = $this->configLocator->locate(TestFrameworkTypes::PHPUNIT);

Expand All @@ -103,7 +113,8 @@ public function create(string $adapterName, bool $skipCoverage): TestFrameworkAd
$this->projectDir,
$this->infectionConfig->getSourceDirectories(),
$skipCoverage,
$this->infectionConfig->getExecuteOnlyCoveringTestCases()
$this->infectionConfig->getExecuteOnlyCoveringTestCases(),
$filteredSourceFilesToMutate
);
}

Expand All @@ -122,7 +133,8 @@ public function create(string $adapterName, bool $skipCoverage): TestFrameworkAd
$this->projectDir,
$this->infectionConfig->getSourceDirectories(),
$skipCoverage,
$this->infectionConfig->getExecuteOnlyCoveringTestCases()
$this->infectionConfig->getExecuteOnlyCoveringTestCases(),
$filteredSourceFilesToMutate
);
}

Expand Down Expand Up @@ -159,4 +171,26 @@ public function create(string $adapterName, bool $skipCoverage): TestFrameworkAd
implode(', ', $availableTestFrameworks)
));
}

/**
* Get only those source files that will be mutated to use them in coverage whitelist
*
* @return list<string>
*/
private function getFilteredSourceFilesToMutate(): array
{
if ($this->sourceFileFilter->getFilters() === []) {
return [];
}

/** @var list<string> $filteredPaths */
$filteredPaths = array_filter(array_map(
static function (SplFileInfo $file) {
return $file->getRealPath();
},
iterator_to_array($this->sourceFileFilter->filter($this->infectionConfig->getSourceFiles()))
));

return $filteredPaths;
}
}
9 changes: 7 additions & 2 deletions src/TestFramework/PhpUnit/Adapter/PestAdapterFactory.php
Expand Up @@ -56,6 +56,9 @@
*/
final class PestAdapterFactory implements TestFrameworkAdapterFactory
{
/**
* @param list<string> $filteredSourceFilesToMutate
*/
public static function create(
string $testFrameworkExecutable,
string $tmpDir,
Expand All @@ -65,7 +68,8 @@ public static function create(
string $projectDir,
array $sourceDirectories,
bool $skipCoverage,
bool $executeOnlyCoveringTestCases = false
bool $executeOnlyCoveringTestCases = false,
array $filteredSourceFilesToMutate = []
): TestFrameworkAdapter {
Assert::string($testFrameworkConfigDir, 'Config dir is not allowed to be `null` for the Pest adapter');

Expand All @@ -89,7 +93,8 @@ public static function create(
$testFrameworkConfigContent,
$configManipulator,
new XmlConfigurationVersionProvider(),
$sourceDirectories
$sourceDirectories,
$filteredSourceFilesToMutate
),
new MutationConfigBuilder(
$tmpDir,
Expand Down
7 changes: 5 additions & 2 deletions src/TestFramework/PhpUnit/Adapter/PhpUnitAdapterFactory.php
Expand Up @@ -58,6 +58,7 @@ final class PhpUnitAdapterFactory implements TestFrameworkAdapterFactory
{
/**
* @param string[] $sourceDirectories
* @param list<string> $filteredSourceFilesToMutate
*/
public static function create(
string $testFrameworkExecutable,
Expand All @@ -68,7 +69,8 @@ public static function create(
string $projectDir,
array $sourceDirectories,
bool $skipCoverage,
bool $executeOnlyCoveringTestCases = false
bool $executeOnlyCoveringTestCases = false,
array $filteredSourceFilesToMutate = []
): TestFrameworkAdapter {
Assert::string($testFrameworkConfigDir, 'Config dir is not allowed to be `null` for the Pest adapter');

Expand All @@ -92,7 +94,8 @@ public static function create(
$testFrameworkConfigContent,
$configManipulator,
new XmlConfigurationVersionProvider(),
$sourceDirectories
$sourceDirectories,
$filteredSourceFilesToMutate
),
new MutationConfigBuilder(
$tmpDir,
Expand Down
15 changes: 10 additions & 5 deletions src/TestFramework/PhpUnit/Config/Builder/InitialConfigBuilder.php
Expand Up @@ -56,16 +56,20 @@ class InitialConfigBuilder implements ConfigBuilder
private XmlConfigurationVersionProvider $versionProvider;
/** @var string[] */
private array $srcDirs;
/** @var list<string> */
private array $filteredSourceFilesToMutate;

/**
* @param string[] $srcDirs
* @param list<string> $filteredSourceFilesToMutate
*/
public function __construct(
string $tmpDir,
string $originalXmlConfigContent,
XmlConfigurationManipulator $configManipulator,
XmlConfigurationVersionProvider $versionProvider,
array $srcDirs
array $srcDirs,
array $filteredSourceFilesToMutate
) {
Assert::notEmpty(
$originalXmlConfigContent,
Expand All @@ -77,6 +81,7 @@ public function __construct(
$this->configManipulator = $configManipulator;
$this->versionProvider = $versionProvider;
$this->srcDirs = $srcDirs;
$this->filteredSourceFilesToMutate = $filteredSourceFilesToMutate;
}

public function build(string $version): string
Expand Down Expand Up @@ -118,25 +123,25 @@ private function buildPath(): string
private function addCoverageNodes(string $version, SafeDOMXPath $xPath): void
{
if (version_compare($version, '10', '>=')) {
$this->configManipulator->addCoverageIncludeNodesUnlessTheyExist($xPath, $this->srcDirs);
$this->configManipulator->addOrUpdateCoverageIncludeNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);

return;
}

if (version_compare($version, '9.3', '<')) {
$this->configManipulator->addLegacyCoverageWhitelistNodesUnlessTheyExist($xPath, $this->srcDirs);
$this->configManipulator->addOrUpdateLegacyCoverageWhitelistNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);

return;
}

// For versions between 9.3 and 10.0, fallback to version provider
if (version_compare($this->versionProvider->provide($xPath), '9.3', '>=')) {
$this->configManipulator->addCoverageIncludeNodesUnlessTheyExist($xPath, $this->srcDirs);
$this->configManipulator->addOrUpdateCoverageIncludeNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);

return;
}

$this->configManipulator->addLegacyCoverageWhitelistNodesUnlessTheyExist($xPath, $this->srcDirs);
$this->configManipulator->addOrUpdateLegacyCoverageWhitelistNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);
}

private function addRandomTestsOrderAttributesIfNotSet(string $version, SafeDOMXPath $xPath): void
Expand Down
2 changes: 1 addition & 1 deletion src/TestFramework/PhpUnit/Config/Path/PathReplacer.php
Expand Up @@ -48,7 +48,7 @@
final class PathReplacer
{
private Filesystem $filesystem;
private ?string $phpUnitConfigDir = null;
private ?string $phpUnitConfigDir;

public function __construct(Filesystem $filesystem, ?string $phpUnitConfigDir = null)
{
Expand Down
71 changes: 55 additions & 16 deletions src/TestFramework/PhpUnit/Config/XmlConfigurationManipulator.php
Expand Up @@ -132,18 +132,20 @@ public function removeExistingPrinters(SafeDOMXPath $xPath): void

/**
* @param string[] $srcDirs
* @param list<string> $filteredSourceFilesToMutate
*/
public function addLegacyCoverageWhitelistNodesUnlessTheyExist(SafeDOMXPath $xPath, array $srcDirs): void
public function addOrUpdateLegacyCoverageWhitelistNodes(SafeDOMXPath $xPath, array $srcDirs, array $filteredSourceFilesToMutate): void
{
$this->addCoverageNodesUnlessTheyExist('filter', 'whitelist', $xPath, $srcDirs);
$this->addOrUpdateCoverageNodes('filter', 'whitelist', $xPath, $srcDirs, $filteredSourceFilesToMutate);
}

/**
* @param string[] $srcDirs
* @param list<string> $filteredSourceFilesToMutate
*/
public function addCoverageIncludeNodesUnlessTheyExist(SafeDOMXPath $xPath, array $srcDirs): void
public function addOrUpdateCoverageIncludeNodes(SafeDOMXPath $xPath, array $srcDirs, array $filteredSourceFilesToMutate): void
{
$this->addCoverageNodesUnlessTheyExist('coverage', 'include', $xPath, $srcDirs);
$this->addOrUpdateCoverageNodes('coverage', 'include', $xPath, $srcDirs, $filteredSourceFilesToMutate);
}

public function validate(string $configPath, SafeDOMXPath $xPath): bool
Expand Down Expand Up @@ -182,24 +184,43 @@ public function removeDefaultTestSuite(SafeDOMXPath $xPath): void

/**
* @param string[] $srcDirs
* @param list<string> $filteredSourceFilesToMutate
*/
private function addCoverageNodesUnlessTheyExist(string $parentName, string $listName, SafeDOMXPath $xPath, array $srcDirs): void
private function addOrUpdateCoverageNodes(string $parentName, string $listName, SafeDOMXPath $xPath, array $srcDirs, array $filteredSourceFilesToMutate): void
{
if ($this->nodeExists($xPath, "{$parentName}/{$listName}")) {
return;
$coverageNodeExists = $this->nodeExists($xPath, "{$parentName}/{$listName}");

if ($coverageNodeExists) {
if ($filteredSourceFilesToMutate === []) {
// use original phpunit.xml's coverage setting since all files need to be mutated (no filter is set)
return;
}

$this->removeCoverageChildNode($xPath, "{$parentName}/{$listName}");
}

$filterNode = $this->createNode($xPath->document, $parentName);
$filterNode = $this->getOrCreateNode($xPath, $xPath->document, $parentName);

$listNode = $xPath->document->createElement($listName);

foreach ($srcDirs as $srcDir) {
$directoryNode = $xPath->document->createElement(
'directory',
$srcDir
);
if ($filteredSourceFilesToMutate === []) {
foreach ($srcDirs as $srcDir) {
$directoryNode = $xPath->document->createElement(
'directory',
$srcDir
);

$listNode->appendChild($directoryNode);
}
} else {
foreach ($filteredSourceFilesToMutate as $sourceFileRealPath) {
$directoryNode = $xPath->document->createElement(
'file',
$sourceFileRealPath
);

$listNode->appendChild($directoryNode);
$listNode->appendChild($directoryNode);
}
}

$filterNode->appendChild($listNode);
Expand Down Expand Up @@ -264,7 +285,7 @@ private function removeAttribute(SafeDOMXPath $xPath, string $name): void
$name
));

if ($nodeList->length) {
if ($nodeList->length > 0) {
$document = $xPath->document->documentElement;
Assert::isInstanceOf($document, DOMElement::class);
$document->removeAttribute($name);
Expand All @@ -278,7 +299,7 @@ private function setAttributeValue(SafeDOMXPath $xPath, string $name, string $va
$name
));

if ($nodeList->length) {
if ($nodeList->length > 0) {
$nodeList[0]->nodeValue = $value;
} else {
$node = $xPath->query('/phpunit')[0];
Expand All @@ -302,4 +323,22 @@ private function getErrorLevelName(LibXMLError $error): string

throw new LogicException(sprintf('Unknown lib XML error level "%s"', $error->level));
}

private function removeCoverageChildNode(SafeDOMXPath $xPath, string $nodeQuery): void
{
foreach ($xPath->query($nodeQuery) as $node) {
$node->parentNode->removeChild($node);
}
}

private function getOrCreateNode(SafeDOMXPath $xPath, DOMDocument $dom, string $nodeName): DOMElement
{
$node = $xPath->query(sprintf('/phpunit/%s', $nodeName));

if ($node->length > 0) {
return $node[0];
}

return $this->createNode($dom, $nodeName);
}
}

0 comments on commit 193ab76

Please sign in to comment.