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

Mutant & Mutation tweaks #1207

Closed
wants to merge 2 commits into from
Closed
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
6 changes: 3 additions & 3 deletions src/Metrics/MetricsCalculator.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class MetricsCalculator
/**
* @var int
*/
private $notCoveredByTestsCount = 0;
private $notExecutedByTestsCount = 0;

/**
* @var int
Expand Down Expand Up @@ -110,7 +110,7 @@ public function collect(MutantExecutionResult ...$executionResults): void
break;

case DetectionStatus::NOT_COVERED:
$this->notCoveredByTestsCount++;
$this->notExecutedByTestsCount++;
Copy link
Member

Choose a reason for hiding this comment

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

I'm -1 on this renaming

  • it's clear what covered means, I don't think there is any confusion in "coverage notion"
  • now it's consfusing why the property is named notExecutedByTestsCount, but the detection status is NOT_COVERED

Copy link
Member Author

Choose a reason for hiding this comment

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

it's clear what covered means,

It's not, because it is not covered. The line code coverage only guarantees that a line is executed by a given test, it does not mean this test covers this line.

Also there was a discussion recently with the other mutation testing tools about an alternative to the kill/escape vocabulary and the resounding candidate was covered/uncovered which is going to be even more confusing if we keep those words with the line coverage

I totally agree with you about the detection status, I would like to change it, but that requires a bit more refactoring affecting the UI as well hence I left it alone for now.

Copy link
Member

Choose a reason for hiding this comment

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

It's not, because it is not covered. The line code coverage only guarantees that a line is executed by a given test, it does not mean this test covers this line.

I don't agree. I know the difference between types of coverage, and PHPUnit has line coverage, which exactly means that line is covered as soon as any of the opcode is executed.

So from PHPUnit point of view - line is covered. And we use only line coverage in PHP

$this->notCoveredExecutionResults->add($executionResult);

break;
Expand Down Expand Up @@ -164,7 +164,7 @@ public function getTimedOutCount(): int

public function getNotTestedCount(): int
{
return $this->notCoveredByTestsCount;
return $this->notExecutedByTestsCount;
}

public function getTotalMutantsCount(): int
Expand Down
7 changes: 1 addition & 6 deletions src/Mutant/Mutant.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,11 @@ public function getDiff(): string
return $this->diff;
}

public function isCoveredByTest(): bool
{
return $this->mutation->isCoveredByTest();
}

/**
* @return TestLocation[]
*/
public function getTests(): array
{
return $this->mutation->getAllTests();
return $this->mutation->getTests();
}
}
2 changes: 1 addition & 1 deletion src/Mutant/MutantExecutionResultFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ private function retrieveProcessOutput(Process $process): string

private function retrieveDetectionStatus(MutantProcess $mutantProcess): string
{
if (!$mutantProcess->getMutant()->isCoveredByTest()) {
if (!$mutantProcess->getMutant()->getMutation()->hasTests()) {
return DetectionStatus::NOT_COVERED;
}

Expand Down
11 changes: 5 additions & 6 deletions src/Mutation/Mutation.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class Mutation
private $attributes;
private $originalFileAst;
private $tests;
private $coveredByTests;
private $executedByTests;

/**
* @var string|null
Expand Down Expand Up @@ -106,7 +106,7 @@ public function __construct(
$this->mutatedNode = $mutatedNode;
$this->mutationByMutatorIndex = $mutationByMutatorIndex;
$this->tests = $tests;
$this->coveredByTests = count($tests) > 0;
$this->executedByTests = count($tests) > 0;
}

public function getOriginalFilePath(): string
Expand Down Expand Up @@ -150,16 +150,15 @@ public function getMutatedNode(): MutatedNode
return $this->mutatedNode;
}

// TODO: hasTest()?
public function isCoveredByTest(): bool
public function hasTests(): bool
Copy link
Member

Choose a reason for hiding this comment

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

same here

{
return $this->coveredByTests;
return $this->executedByTests;
}

/**
* @return TestLocation[]
*/
public function getAllTests(): array
public function getTests(): array
{
return $this->tests;
}
Expand Down
8 changes: 5 additions & 3 deletions src/Process/Builder/MutantProcessBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,14 @@ public function __construct(

public function createProcessForMutant(Mutant $mutant, string $testFrameworkExtraOptions = ''): MutantProcess
{
$mutation = $mutant->getMutation();

$process = new Process(
$this->testFrameworkAdapter->getMutantCommandLine(
$mutant->getTests(),
$mutation->getTests(),
$mutant->getFilePath(),
$mutant->getMutation()->getHash(),
$mutant->getMutation()->getOriginalFilePath(),
$mutation->getHash(),
$mutation->getOriginalFilePath(),
$testFrameworkExtraOptions
)
);
Expand Down
7 changes: 5 additions & 2 deletions src/Process/Runner/MutationTestingRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,11 @@ public function run(iterable $mutations, string $testFrameworkExtraOptions): voi
return $this->mutantFactory->create($mutation);
})
->filter(function (Mutant $mutant) {
// It's a proxy call to Mutation, can be done one stage up
if ($mutant->isCoveredByTest()) {
// The filtering is done here since with a mutant and not earlier with a mutation
// since:
// - if pass the filtering, the mutant is going to be used
// - if does not pass the filtering, the mutant is used for the reports
if ($mutant->getMutation()->hasTests()) {
return true;
}

Expand Down
10 changes: 1 addition & 9 deletions tests/phpunit/Mutant/MutantAssertions.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,21 @@

namespace Infection\Tests\Mutant;

use Infection\AbstractTestFramework\Coverage\TestLocation;
use Infection\Mutant\Mutant;
use Infection\Mutation\Mutation;

trait MutantAssertions
{
/**
* @param TestLocation[] $expectedTests
*/
public function assertMutantStateIs(
Mutant $mutant,
string $expectedFilePath,
Mutation $expectedMutation,
string $expectedMutatedCode,
string $expectedDiff,
bool $expectedCoveredByTests,
array $expectedTests
string $expectedDiff
): void {
$this->assertSame($expectedFilePath, $mutant->getFilePath());
$this->assertSame($expectedMutation, $mutant->getMutation());
$this->assertSame($expectedMutatedCode, $mutant->getMutatedCode());
$this->assertSame($expectedDiff, $mutant->getDiff());
$this->assertSame($expectedCoveredByTests, $mutant->isCoveredByTest());
$this->assertSame($expectedTests, $mutant->getTests());
}
}
12 changes: 2 additions & 10 deletions tests/phpunit/Mutant/MutantFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,7 @@ public function test_it_creates_a_mutant_instance_from_the_given_mutation(): voi
new Node\Name('Acme'),
[new Node\Scalar\LNumber(0)]
)],
$tests = [
new TestLocation(
'FooTest::test_it_can_instantiate',
'/path/to/acme/FooTest.php',
0.01
),
]
[]
);

$expectedMutantFilePath = sprintf(
Expand Down Expand Up @@ -140,9 +134,7 @@ public function test_it_creates_a_mutant_instance_from_the_given_mutation(): voi
$expectedMutantFilePath,
$mutation,
'mutated code',
'code diff',
true,
$tests
'code diff'
);
}

Expand Down
14 changes: 2 additions & 12 deletions tests/phpunit/Mutant/MutantTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,12 @@ final class MutantTest extends TestCase

/**
* @dataProvider valuesProvider
*
* @param TestLocation[] $expectedTests
*/
public function test_it_can_be_instantiated(
string $filePath,
Mutation $mutation,
string $mutatedCode,
string $diff,
bool $expectedCoveredByTests,
array $expectedTests
string $diff
): void {
$mutant = new Mutant($filePath, $mutation, $mutatedCode, $diff);

Expand All @@ -68,9 +64,7 @@ public function test_it_can_be_instantiated(
$filePath,
$mutation,
$mutatedCode,
$diff,
$expectedCoveredByTests,
$expectedTests
$diff
);
}

Expand Down Expand Up @@ -110,8 +104,6 @@ public function valuesProvider(): iterable
),
'mutated code',
'diff value',
true,
$tests,
];

yield 'nominal without tests' => [
Expand All @@ -131,8 +123,6 @@ public function valuesProvider(): iterable
),
'mutated code',
'diff value',
false,
[],
];
}
}
6 changes: 3 additions & 3 deletions tests/phpunit/Mutation/MutationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public function test_it_can_be_instantiated(
array $tests,
array $expectedAttributes,
int $expectedOriginalStartingLine,
bool $expectedCoveredByTests,
bool $expectedHasTests,
string $expectedHash
): void {
$mutation = new Mutation(
Expand All @@ -87,8 +87,8 @@ public function test_it_can_be_instantiated(
$this->assertSame($expectedOriginalStartingLine, $mutation->getOriginalStartingLine());
$this->assertSame($mutatedNodeClass, $mutation->getMutatedNodeClass());
$this->assertSame($mutatedNode, $mutation->getMutatedNode());
$this->assertSame($tests, $mutation->getAllTests());
$this->assertSame($expectedCoveredByTests, $mutation->isCoveredByTest());
$this->assertSame($tests, $mutation->getTests());
$this->assertSame($expectedHasTests, $mutation->hasTests());
$this->assertSame($expectedHash, $mutation->getHash());
}

Expand Down