Plugin system to allow pre-/post- filtering and modification of mutations

[Ocramius]

Scope - third-party tools integration

The idea is relatively simple: some mutations don't make sense, according to other tools that are completely third-party to infection.

Such tools could say things like:

• == cannot exist, according to our coding rules

• we never negate conditionals, because it is confusing to us

• we have strict types

• all methods must be public (silly - please don't do this, but just here for the sake of the example)

Hacks done so far

I've tinkered a lot on https://github.com/Roave/infection-static-analysis-plugin last week.

The tool currently operates through AOP by replacing the Infection\Mutant\MutantExecutionResultFactory service. Whenever a mutant result is produced, we analyze it, and decide whether it is valid as an escaped mutant through a third-party tool.

If the third-party tool says that the mutation is invalid, it is marked as KILLED.

Wished API

Plugins can be either very lightweight (#697, for example, is about adding a regex to a mutation) or very heavyweight (running complex static analysis, fuzzy-testing or property tests on top of a mutation).

Therefore, it makes sense to allow both pre- and post- filtering of mutations.

Pre-filtering

The ideal location where pre-filtering could be done is inside Infection\Process\Runner\MutationTestingRunner#run():

• infection/src/Process/Runner/MutationTestingRunner.php

  Lines 90 to 124 in 25582f2

  	$processes = take($mutations)
  	->map(function (Mutation $mutation): Mutant {
  	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()) {
  	return true;
  	}
  	$this->eventDispatcher->dispatch(new MutantProcessWasFinished(
  	MutantExecutionResult::createFromNonCoveredMutant($mutant)
  	));
  	return false;
  	})
  	->filter(function (Mutant $mutant) {
  	if ($mutant->getMutation()->getNominalTestExecutionTime() < $this->timeout) {
  	return true;
  	}
  	$this->eventDispatcher->dispatch(new MutantProcessWasFinished(
  	MutantExecutionResult::createFromTimeSkippedMutant($mutant)
  	));
  	return false;
  	})
  	->map(function (Mutant $mutant) use ($testFrameworkExtraOptions): ProcessBearer {
  	$this->fileSystem->dumpFile($mutant->getFilePath(), $mutant->getMutatedCode());
  	$process = $this->processFactory->createProcessForMutant($mutant, $testFrameworkExtraOptions);
  	return $process;
  	})
  	;

Here, we could have a replaceable filter for mutations with a signature like:

@psalm-type PreFilterMutant callable(Mutant $mutant): bool {}

or

preFilterMutant :: Mutant -> IO Bool

(reason for IO here is that external tools are gonna be shitty anyway: embrace the shittyness)

This is where static analysis could be plugged in, or #697 could be solved: invalid code mutations can be filtered with a very quick operation, saving a ton of CPU before runtime.

Note: we could also ->map() here, to allow replacing a Mutant

Post-filtering

Post-filtering is more tricky, because of how mutations are considered "done", and how mutation execution progress is tracked.

This happens through relatively ugly callbacks in Infection\Process\Runner\MutationTestingRunner#run():

infection/src/Process/Runner/MutationTestingRunner.php

Lines 117 to 128 in 25582f2

	->map(function (Mutant $mutant) use ($testFrameworkExtraOptions): ProcessBearer {
	$this->fileSystem->dumpFile($mutant->getFilePath(), $mutant->getMutatedCode());
	$process = $this->processFactory->createProcessForMutant($mutant, $testFrameworkExtraOptions);
	return $process;
	})
	;
	$this->processRunner->run($processes);
	$this->eventDispatcher->dispatch(new MutationTestingWasFinished());

In Infection\Process\Factory\MutantProcessFactory#createProcessForMutant() this becomes even more muddy:

infection/src/Process/Factory/MutantProcessFactory.php

Lines 95 to 106 in 25582f2

	$mutantProcess->registerTerminateProcessClosure(static function () use (
	$mutantProcess,
	$eventDispatcher,
	$resultFactory
	): void {
	$eventDispatcher->dispatch(new MutantProcessWasFinished(
	$resultFactory->createFromProcess($mutantProcess))
	);
	});
	return $mutantProcess;
	}

This is very un-manageable: I think it would make sense to use something like an Observable (/cc @WyriHaximus if you can help out with a suggestion here) to which we publish the completion of the process, rather than publishing to an event dispatcher.

We can then filter said observable exactly like we did with the pre-filtering above:

@psalm-type PostFilterMutant callable(MutantProcess $mutant): bool {}

or

postFilterMutant :: MutantProcess -> IO Bool

Then:

$completedMutationRuns->filter(function (MutantProcess $process): bool {
    return 1 === \preg_match('/potato/', $process->getCommandLine()); // don't show potatoes in the result
});

Note: we could also ->map() here, to allow replacing a MutantProcess

@internal and backward compatibility promise

Few things in this issue require some added maintenance effort for @infection maintainers:

• Some of the Container API needs to become public: specifically for setting a pre- and post- filter callback:
  

  infection/src/Container.php

  Lines 144 to 147 in 25582f2

  	/**
  	* @internal
  	*/
  	final class Container

• Mutant needs to become public API to some degree
  

  infection/src/Mutant/Mutant.php

  Lines 41 to 45 in 25582f2

  	/**
  	* @internal
  	* @final
  	*/
  	class Mutant

• Some of the MutantProcess API needs to become public
  

  infection/src/Process/MutantProcess.php

  Lines 43 to 47 in 25582f2

  	/**
  	* @internal
  	* @final
  	*/
  	class MutantProcess implements ProcessBearer

That means that before working on this, there needs to be a clear idea by maintainers of the project whether those symbols can be made public, or whether more work needs to be done around them first (https://twitter.com/tfidry/status/1302896716065144833 /cc @theofidry )

[WyriHaximus]

This is very un-manageable: I think it would make sense to use something like an Observable (/cc @WyriHaximus if you can help out with a suggestion here) to which we publish the completion of the process, rather than publishing to an event dispatcher.

Using RxPHP you could kick it off like (dropping the need for an event loop from the example):

$filteredMutations = [];
Observable::fromArray(
    $arrayOfMutations, 
    Scheduler::getImmediate()
)->filter(
    static fn (Mutation $mutation): bool => $mutation === $yay
)->toArray()->subscribe(
    static function (array $mutations) use (&$filteredMutations): void
    {
        $filteredMutations = $mutations;
    }
);
return $filteredMutations;

[sanmai]

If the third-party tool says that the mutation is invalid, it is marked as KILLED.

I'd argue it'll be better to mark these as skipped, as we have this convenient status. Else they will skew the metrics towards better numbers.

[Ocramius]

I'd argue it'll be better to mark these as skipped

They're 100% killed, since the mutation is covered by some sort of formal verification (in my case, static analysis).

Alternatively, they can be completely removed (not show up in any report at all) if we manage to build pre-filtering, but "skipped" would indicate that something needs to be done, which isn't the case.

[theofidry]

What's the goal of the post-filtering exactly?

[sanmai]

What is the specific use case for the post-filtering? As I see it, if something can be done during post-filtering, it can be done during pre-filtering. Is it not?

[Ocramius]

@theofidry @sanmai performing heavy operations that are only worth doing on escaped mutants, for example.

An example could be to run pre-existing property testing (expensive) on a mutant, but right now, static analysis is also quite slow for most unit test suites.

[theofidry]

IMO that would require a lot more than a post-filtering hook then. Because the current suggested hook provides a closed (already executed) process and not much more and is currently only used to create the results objects which are used for logging purposes – not something that offers much room for meddling atm.

What you are suggesting, if I understand correctly, looks more fit to me as a "retry" or "complement execution" strategy for escaped mutants which could be interested but indeed requires a few architectural changes

[Ocramius]

Looking back at this, inplementing pre-filtering/mapping as a start would suffice, leaving post-filtering/mapping for a later point in time.

That's already solving all linked issues (for now)

[sanmai]

@Ocramius if you have a moment can you see if the API from #1327 would work for you?
It should be runnable in the current state.

[VincentLanglet]

This would be a great feature,

For instance it would avoid having mutation like

         $content = file_get_contents($this->file);
-        if (false === $content) {
+        if (true === $content) {

reported as uncovered when using phpstan/psalm