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

[Performance] Add files to coverage whitelist instead of the whole directories when --filter or --git-diff-filter are used #1543

Merged
merged 3 commits into from Aug 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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;
Copy link
Member Author

Choose a reason for hiding this comment

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

@sanmai FYI: I didn't find any test or case that will be broken by this change, I think we've added it just in case to catch the situation where Finder is used twice. But in this PR - this is exactly what is needed

Copy link
Member

Choose a reason for hiding this comment

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

Might as well change the signature to @return array<...>

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;
Copy link
Member Author

Choose a reason for hiding this comment

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

useless default value since this property is initialized in constructor

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);
}
}