Syntax errors are not accounted for, not counted

[sanmai]

As of writing Infection does not distinguish between different types of errors, of which there are at least several groups:

• Common language errors (Error: Call to protected method)
• Memory errors (Fatal error: Allowed memory size of, see Heuristics to set memory limit for mutants running with PHPUnit #258)
• Syntax errors (we should not have them, but how do we know if we do not account for them)
• File errors (e.g. flock(): Illegal operation argument could be a temporary glitch)
• Other errors

That's just some types that I think worth counting and reporting separately. There could be more.

First reported here.

Related: #222

[maks-rafalko]

Syntax errors (we should not have them, but how do we know if we do not account for them)

I thought PHP-Parser should always generate valid PHP code, but can't find a proof in its documentation anymore. Probably I mixed up something.

Also related to #222 (comment) (see proposed report):

6 mutations were generated:
   5 mutants were killed:
       - 5 tests failures
       - 0 PHP errors
       - 0 time outs

So we can add those groups to the report above

[sanmai]

I thought PHP-Parser should always generate valid PHP code

Well, we can safely say that it is proven that it doesn't always generate a correct code.

Errors from mutators from #243 have:

-        $updater->getStrategy()->setPackageName(self::PACKAGE_NAME);
+        false->setPackageName(self::PACKAGE_NAME);

And that is a parse error: https://3v4l.org/kZNGg

PHP Parse error:  syntax error, unexpected '->' (T_OBJECT_OPERATOR)

Therefore we must account for syntax errors.

[theofidry]

I think we should try to be smarter and account for useless/equivalent mutants at some point.

To expand a bit: creating a mutant is relatively easy and requires little resources. However evaluating a mutant is way more expansive (we need to run the tests). There is a report somewhere mentioning about ~30% of the generated mutants are redundant hence not useful. I still need to check the effective strategies we could employ, but meanwhile checking for a PHP validity would be a good start and removing a bunch of mutants that provide no value.

[BackEndTea]

What if we could lint the 'new' PHP before writing it to a file? And if it contains a syntax error ignore it.

[maks-rafalko]

Well, as @sanmai already pointed

we should not have them (syntax errors)

My opinion is we should do our best to not allow Mutators produce invalid syntax. That's it.

If it requires a complex code - no problem, we will have to write it, but not leave mutators that produce invalid syntax (related to this). So we should fix the root of the problem rather than linting the code and ignore invalid created Mutant.

Linting the code

• requires another dependency
• slows down infection
• will lint every Mutant that has always valid syntax (e.g. === -> !==) which is useless wasting of resources

---

Created Mutant with invalid syntax must be considered as a bug in Infection.

[sanmai]

$ find src/ -type f | wc -l
190
$ time find src/ -type f -exec php -l {} \;
No syntax errors detected in src/....

real	0m10.120s
user	0m6.160s
sys	0m3.093s

That's 0.05 seconds per file on average, or 0.05 seconds per every mutation. Or 50 seconds for each 1000 mutations extra to what it currently takes.

No matter how much I like knowing exactly if we caused a syntax error (and hit a bug), considering that most often we won't be dealing with syntax errors, it isn't seemingly worth it to lint every file specifically outside of the usual testing process.

[theofidry]

@sanmai remember that the time taken for each mutation greatly differs from one project to another, and it can be hella slow.

That said I agree with the thought that if the linting process is too expansive, we're better of not doing it...

[sanmai]

Other than our own needs being addressed #301, I'm sure PHPUnit can be made to report fatal errors with some twiddling with phpunit.xml we use for our runners, and we can parse the errors just like we're doing it now. (For that we would need a "broken" mutator or two, that will always produce a code with a syntax error. Next is E2E test case with this mutator, and so on with detection.)

[maks-rafalko]

In addition to the last comment and for future implementator:

we can parse the test framework output here and add new execution result like SYNTAX_ERROR

infection/src/Mutant/MutantExecutionResultFactory.php

Lines 89 to 110 in 2a1a91a

	private function retrieveDetectionStatus(MutantProcess $mutantProcess): string
	{
	if (!$mutantProcess->getMutant()->isCoveredByTest()) {
	return DetectionStatus::NOT_COVERED;
	}
	if ($mutantProcess->isTimedOut()) {
	return DetectionStatus::TIMED_OUT;
	}
	$process = $mutantProcess->getProcess();
	if ($process->getExitCode() > 100) {
	// See \Symfony\Component\Process\Process::$exitCodes
	return DetectionStatus::ERROR;
	}
	if ($this->testFrameworkAdapter->testsPass($this->retrieveProcessOutput($process))) {
	return DetectionStatus::ESCAPED;
	}
	return DetectionStatus::KILLED;

we can add a new interface and assing it to those test frameworks that can/should parse syntax error (implementation of parsers will be different and depend on test framework)