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.

Sign up for GitHub

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

Jump to bottom

Request coverage reports with command line args #1375

Merged
merged 5 commits into from
Oct 26, 2020
Merged

Request coverage reports with command line args #1375

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5765c6d
Request coverage reports with command line args
sanmai Oct 25, 2020
2fad88a
Just use forward slash
sanmai Oct 25, 2020
3e0a645
Remove coverage reports from PHPUnit 9.3+ format
sanmai Oct 25, 2020
838441c
Update tests/phpunit/TestFramework/PhpUnit/Config/Builder/InitialConf…
sanmai Oct 26, 2020
1f30aab
Comment for removeExistingLoggers
sanmai Oct 26, 2020
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
12 changes: 5 additions & 7 deletions src/TestFramework/AbstractTestFrameworkAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@ abstract class AbstractTestFrameworkAdapter implements TestFrameworkAdapter
private $mutationConfigBuilder;
private $versionParser;
private $commandLineBuilder;

/**
* @var string|null
*/
private $version;

public function __construct(
Expand All @@ -65,14 +61,16 @@ public function __construct(
MutationConfigBuilder $mutationConfigBuilder,
CommandLineArgumentsAndOptionsBuilder $argumentsAndOptionsBuilder,
VersionParser $versionParser,
CommandLineBuilder $commandLineBuilder
CommandLineBuilder $commandLineBuilder,
?string $version = null
) {
$this->testFrameworkExecutable = $testFrameworkExecutable;
$this->initialConfigBuilder = $initialConfigBuilder;
$this->mutationConfigBuilder = $mutationConfigBuilder;
$this->argumentsAndOptionsBuilder = $argumentsAndOptionsBuilder;
$this->versionParser = $versionParser;
$this->commandLineBuilder = $commandLineBuilder;
$this->version = $version;
}

abstract public function testsPass(string $output): bool;
Expand All @@ -82,7 +80,7 @@ abstract public function getName(): string;
abstract public function hasJUnitReport(): bool;

/**
* Returns array of arguments to pass them into the Initial Run Symfony Process
* Returns array of arguments to pass them into the Initial Run Process
*
* @param string[] $phpExtraArgs
*
Expand All @@ -97,7 +95,7 @@ public function getInitialTestRunCommandLine(
}

/**
* Returns array of arguments to pass them into the Mutant Symfony Process
* Returns array of arguments to pass them into the Mutant Process
*
* @param TestLocation[] $tests
*
Expand Down
3 changes: 2 additions & 1 deletion src/TestFramework/CommandLineBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@

/**
* @internal
* @final
*/
final class CommandLineBuilder
class CommandLineBuilder
{
/**
* @var string[]|null
Expand Down
50 changes: 50 additions & 0 deletions src/TestFramework/PhpUnit/Adapter/PhpUnitAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,13 @@
use Infection\AbstractTestFramework\MemoryUsageAware;
use Infection\PhpParser\Visitor\IgnoreNode\PhpUnitCodeCoverageAnnotationIgnorer;
use Infection\TestFramework\AbstractTestFrameworkAdapter;
use Infection\TestFramework\CommandLineArgumentsAndOptionsBuilder;
use Infection\TestFramework\CommandLineBuilder;
use Infection\TestFramework\Config\InitialConfigBuilder;
use Infection\TestFramework\Config\MutationConfigBuilder;
use Infection\TestFramework\IgnoresAdditionalNodes;
use Infection\TestFramework\ProvidesInitialRunOnlyOptions;
use Infection\TestFramework\VersionParser;
use function Safe\preg_match;
use function Safe\sprintf;
use function version_compare;
Expand All @@ -52,11 +57,56 @@ class PhpUnitAdapter extends AbstractTestFrameworkAdapter implements IgnoresAddi
{
public const COVERAGE_DIR = 'coverage-xml';

private $tmpDir;

private $jUnitFilePath;

public function __construct(
string $testFrameworkExecutable,
string $tmpDir,
string $jUnitFilePath,
InitialConfigBuilder $initialConfigBuilder,
MutationConfigBuilder $mutationConfigBuilder,
CommandLineArgumentsAndOptionsBuilder $argumentsAndOptionsBuilder,
VersionParser $versionParser,
CommandLineBuilder $commandLineBuilder,
?string $version = null
) {
parent::__construct($testFrameworkExecutable, $initialConfigBuilder, $mutationConfigBuilder, $argumentsAndOptionsBuilder, $versionParser, $commandLineBuilder, $version);

$this->tmpDir = $tmpDir;
$this->jUnitFilePath = $jUnitFilePath;
}
Copy link
Member Author

@sanmai sanmai Oct 26, 2020 •
edited

Choose a reason for hiding this comment

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

Extending constructors does not break the LSP.


public function hasJUnitReport(): bool
{
return true;
}

/**
* Returns array of arguments to pass them into the Initial Run Process
*
* @param string[] $phpExtraArgs
*
* @return string[]
*/
public function getInitialTestRunCommandLine(
string $extraOptions,
array $phpExtraArgs,
bool $skipCoverage
): array {
if ($skipCoverage === false) {
Copy link
Member

Choose a reason for hiding this comment

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

question: is it any better than !$skiptCoverage? Found a couple of places and all ones from you. I mean we usually use !$condition with booleans, probably worth unifying

Copy link
Member Author

Choose a reason for hiding this comment

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

For one, !$condition is messy, it might trigger on a null. For another, use === is more generic to all types, more uniform. What do you think?

Is there a fixer for !$bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was very hesitant about === if you remember, and I'm still not exactly on board with the arguments I've seen back then. True, it can be ugly when comparing strings and numbers, for example, but the real reason I'm using === everywhere effectively out of habit now because it prevents unnecessary and avoidable BC breaks.

It is not like someone would extend the very this class, but if this happens, identical comparison gets us prepared. For example, if you were using !$condition and someone extending extending this class decides put a different falsy value here, you can't really change it to something else without a BC break. Say, you want to give a null special meaning? Well, you can't, because null is falsy. On the contrary, if you use $condition === false, you're protected from said BC breaks. Add any nulls anytime you want! That's the reason. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

I know the reason why it looks weird for me, because I've been using phpstan with strict rules for 2 years on my job, and it's impossible to use non-boolean comparison there.

Look at this code with strict rules:

$null = rand(0, 1) === 0 ? null : true;
		
if (!$null) {
			
}

error: Only booleans are allowed in a negated boolean, true|null given.. So if it's nullable - makes sense to use === true, but when it's bool only - it's safe to use just !$null here.

Regarding your example, I guess it's impossible to do it: https://phpstan.org/r/b07764ac-22c8-4c06-ad83-a19ba9e09381 - we can pass only booleans to the parent (original) method.

Anyway, it's not critical, we can leave it. Just not usual usage for me, that's why I asked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. It's nice to have PHPStan warn you about improper negation, but it's a liability nevertheless. E.g. where negation can be improper, === is always safe to use.

$extraOptions = trim(sprintf(
'%s --coverage-xml=%s --log-junit=%s',
$extraOptions,
$this->tmpDir . '/' . self::COVERAGE_DIR,
$this->jUnitFilePath // escapeshellarg() is done up the stack in ArgumentsAndOptionsBuilder
));
}

return parent::getInitialTestRunCommandLine($extraOptions, $phpExtraArgs, $skipCoverage);
}

public function testsPass(string $output): bool
{
if (preg_match('/failures!/i', $output)) {
Expand Down
6 changes: 3 additions & 3 deletions src/TestFramework/PhpUnit/Adapter/PhpUnitAdapterFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ public static function create(

return new PhpUnitAdapter(
$testFrameworkExecutable,
$tmpDir,
$jUnitFilePath,
new InitialConfigBuilder(
$tmpDir,
$testFrameworkConfigContent,
$configManipulator,
$jUnitFilePath,
$sourceDirectories,
$skipCoverage
$sourceDirectories
),
new MutationConfigBuilder(
$tmpDir,
Expand Down
2 changes: 1 addition & 1 deletion src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function build(string $configPath, string $extraOptions): array
'--configuration',
$configPath,
],
explode(' ', $extraOptions)
explode(' ', $extraOptions) // FIXME might break space-containing paths
Copy link
Member Author

@sanmai sanmai Oct 26, 2020 •
edited

Choose a reason for hiding this comment

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

Not fixing this issue in this PR, long enough already.

);

return array_filter($options);
Expand Down
48 changes: 1 addition & 47 deletions src/TestFramework/PhpUnit/Config/Builder/InitialConfigBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
use DOMElement;
use DOMNode;
use Infection\TestFramework\Config\InitialConfigBuilder as ConfigBuilder;
use Infection\TestFramework\PhpUnit\Adapter\PhpUnitAdapter;
use Infection\TestFramework\PhpUnit\Config\XmlConfigurationManipulator;
use Infection\TestFramework\SafeDOMXPath;
use function Safe\file_put_contents;
Expand All @@ -55,9 +54,7 @@ class InitialConfigBuilder implements ConfigBuilder
private $tmpDir;
private $originalXmlConfigContent;
private $configManipulator;
private $jUnitFilePath;
private $srcDirs;
private $skipCoverage;

/**
* @param string[] $srcDirs
Expand All @@ -66,9 +63,7 @@ public function __construct(
string $tmpDir,
string $originalXmlConfigContent,
XmlConfigurationManipulator $configManipulator,
string $jUnitFilePath,
array $srcDirs,
bool $skipCoverage
array $srcDirs
) {
Assert::notEmpty(
$originalXmlConfigContent,
Expand All @@ -78,9 +73,7 @@ public function __construct(
$this->tmpDir = $tmpDir;
$this->originalXmlConfigContent = $originalXmlConfigContent;
$this->configManipulator = $configManipulator;
$this->jUnitFilePath = $jUnitFilePath;
$this->srcDirs = $srcDirs;
$this->skipCoverage = $skipCoverage;
}

public function build(string $version): string
Expand Down Expand Up @@ -109,11 +102,6 @@ public function build(string $version): string
$this->configManipulator->removeExistingLoggers($xPath);
$this->configManipulator->removeExistingPrinters($xPath);

if (!$this->skipCoverage) {
$this->addCodeCoverageLogger($xPath);
$this->addJUnitLogger($xPath);
}

file_put_contents($path, $dom->saveXML());

return $path;
Expand All @@ -124,28 +112,6 @@ private function buildPath(): string
return $this->tmpDir . '/phpunitConfiguration.initial.infection.xml';
}

private function addJUnitLogger(SafeDOMXPath $xPath): void
{
$logging = $this->getOrCreateNode($xPath, 'logging');

$junitLog = $xPath->document->createElement('log');
$junitLog->setAttribute('type', 'junit');
$junitLog->setAttribute('target', $this->jUnitFilePath);

$logging->appendChild($junitLog);
}

private function addCodeCoverageLogger(SafeDOMXPath $xPath): void
{
$logging = $this->getOrCreateNode($xPath, 'logging');

$coverageXmlLog = $xPath->document->createElement('log');
$coverageXmlLog->setAttribute('type', 'coverage-xml');
$coverageXmlLog->setAttribute('target', $this->tmpDir . '/' . PhpUnitAdapter::COVERAGE_DIR);

$logging->appendChild($coverageXmlLog);
}

private function addCoverageFilterWhitelistIfDoesNotExist(SafeDOMXPath $xPath): void
{
$filterNode = $this->getNode($xPath, 'filter');
Expand All @@ -168,18 +134,6 @@ private function addCoverageFilterWhitelistIfDoesNotExist(SafeDOMXPath $xPath):
}
}

private function getOrCreateNode(SafeDOMXPath $xPath, string $nodeName): DOMElement
{
$node = $this->getNode($xPath, $nodeName);

if (!$node) {
$node = $this->createNode($xPath->document, $nodeName);
}
Assert::isInstanceOf($node, DOMElement::class);

return $node;
}

private function getNode(SafeDOMXPath $xPath, string $nodeName): ?DOMNode
{
$nodeList = $xPath->query(sprintf('/phpunit/%s', $nodeName));
Expand Down
8 changes: 5 additions & 3 deletions src/TestFramework/PhpUnit/Config/XmlConfigurationManipulator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,11 @@ public function replaceWithAbsolutePaths(SafeDOMXPath $xPath): void
public function removeExistingLoggers(SafeDOMXPath $xPath): void
{
foreach ($xPath->query('/phpunit/logging') as $node) {
$document = $xPath->document->documentElement;
Assert::isInstanceOf($document, DOMElement::class);
$document->removeChild($node);
$node->parentNode->removeChild($node);
maks-rafalko marked this conversation as resolved.
Show resolved Hide resolved
sanmai marked this conversation as resolved.
Show resolved Hide resolved
}

foreach ($xPath->query('/phpunit/coverage/report') as $node) {
$node->parentNode->removeChild($node);
}
}

Expand Down
93 changes: 86 additions & 7 deletions tests/phpunit/TestFramework/PhpUnit/Adapter/PhpUnitAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,28 @@ final class PhpUnitAdapterTest extends TestCase
*/
private $adapter;

private $initialConfigBuilder;
private $mutationConfigBuilder;
private $cliArgumentsBuilder;
private $commandLineBuilder;

protected function setUp(): void
{
$initialConfigBuilder = $this->createMock(InitialConfigBuilder::class);
$mutationConfigBuilder = $this->createMock(MutationConfigBuilder::class);
$cliArgumentsBuilder = $this->createMock(CommandLineArgumentsAndOptionsBuilder::class);
$this->initialConfigBuilder = $this->createMock(InitialConfigBuilder::class);
$this->mutationConfigBuilder = $this->createMock(MutationConfigBuilder::class);
$this->cliArgumentsBuilder = $this->createMock(CommandLineArgumentsAndOptionsBuilder::class);
$this->commandLineBuilder = $this->createMock(CommandLineBuilder::class);

$this->adapter = new PhpUnitAdapter(
'/path/to/phpunit',
$initialConfigBuilder,
$mutationConfigBuilder,
$cliArgumentsBuilder,
'/tmp',
'/tmp/infection/junit.xml',
$this->initialConfigBuilder,
$this->mutationConfigBuilder,
$this->cliArgumentsBuilder,
new VersionParser(),
new CommandLineBuilder()
$this->commandLineBuilder,
'9.0'
);
}

Expand Down Expand Up @@ -120,6 +129,76 @@ public function test_it_provides_node_ignorers(): void
);
}

/**
* @group integration
*/
public function test_it_provides_initial_test_run_command_line_when_no_coverage_is_expected(): void
{
$this->cliArgumentsBuilder
->expects($this->once())
->method('build')
->with('', '--group=default')
;

$this->commandLineBuilder
->expects($this->once())
->method('build')
->with('/path/to/phpunit', ['-d', 'memory_limit=-1'], [])
->willReturn(['/path/to/phpunit', '--dummy-argument'])
;

$initialTestRunCommandLine = $this->adapter->getInitialTestRunCommandLine('--group=default', ['-d', 'memory_limit=-1'], true);

$this->assertSame(
[
'/path/to/phpunit',
'--dummy-argument',
],
$initialTestRunCommandLine
);
}

/**
* @group integration
*/
public function test_it_provides_initial_test_run_command_line_when_coverage_report_is_requested(): void
{
$this->cliArgumentsBuilder
->expects($this->once())
->method('build')
->with('', '--group=default --coverage-xml=/tmp/coverage-xml --log-junit=/tmp/infection/junit.xml')
->willReturn([
'--group=default', '--coverage-xml=/tmp/coverage-xml', '--log-junit=/tmp/infection/junit.xml',
])
;

$this->commandLineBuilder
->expects($this->once())
->method('build')
->with('/path/to/phpunit', ['-d', 'memory_limit=-1'], [
'--group=default', '--coverage-xml=/tmp/coverage-xml', '--log-junit=/tmp/infection/junit.xml',
])
->willReturn([
'/path/to/phpunit',
'--group=default',
'--coverage-xml=/tmp/coverage-xml',
'--log-junit=/tmp/infection/junit.xml',
])
;

$initialTestRunCommandLine = $this->adapter->getInitialTestRunCommandLine('--group=default', ['-d', 'memory_limit=-1'], false);

$this->assertSame(
[
'/path/to/phpunit',
'--group=default',
'--coverage-xml=/tmp/coverage-xml',
'--log-junit=/tmp/infection/junit.xml',
],
$initialTestRunCommandLine
);
}

public function outputProvider(): iterable
{
yield ['OK, but incomplete, skipped, or risky tests!', true];
Expand Down