Difference between PHPUnit <8 and 8+ with fatal errors and exit codes

[maks-rafalko]

@maks-rafalko can you tell why there was an erroring mutation?

Because there was an error in the log.

Before:

Escaped mutants:
================

Timed Out mutants:
==================

Killed mutants:
===============

Errors mutants:
===============


1) /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/src/SourceClass.php:7    [M] PublicVisibility
'/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/phpunit' '--configuration' '/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/./infection/phpunitConfiguration.f9968f13232bd9033816281599b1829e.infection.xml'
--- Original
+++ New
@@ @@
 
 class SourceClass
 {
-    public function hello() : string
+    protected function hello() : string
     {
         return 'hello';
     }
 }
PHPUnit 7.3.0 by Sebastian Bergmann and contributors.

E
Fatal error: Uncaught Exception: One of the tests were skipped. Something wrong with the order of executed tests. in /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/tests/SourceClassTest.php:27
Stack trace:
#0 /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/src/Framework/TestSuite.php(755): PhpUnit_Depends_Order\Test\SourceClassTest::tearDownAfterClass()
#1 /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/src/Framework/TestSuite.php(750): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#2 /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(587): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#3 /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/src/TextUI/Command.php(203): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\ in /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/tests/SourceClassTest.php on line 27

Not Covered mutants:
====================

And it was ok (IMO).

But now something has changed in PHPUnit as we get a different ouput for the same e2e script

After:

Escaped mutants:
================

Timed Out mutants:
==================

Killed mutants:
===============


1) /infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/src/SourceClass.php:7    [M] PublicVisibility
'/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/vendor/phpunit/phpunit/phpunit' '--configuration' '/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/./infection/phpunitConfiguration.68010bbcfc18dfe222fe48dfd3e05470.infection.xml'
--- Original
+++ New
@@ @@
 
 class SourceClass
 {
-    public function hello() : string
+    protected function hello() : string
     {
         return 'hello';
     }
 }
PHPUnit 8.2.4 by Sebastian Bergmann and contributors.

EF

Time: 89 ms, Memory: 4.00 MB

There was 1 error:

1) PhpUnit_Depends_Order\Test\SourceClassTest::test_hello
Error: Call to protected method PhpUnit_Depends_Order\SourceClass::hello() from context 'PhpUnit_Depends_Order\Test\SourceClassTest'

/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/tests/SourceClassTest.php:34

--

There was 1 failure:

1) PhpUnit_Depends_Order\Test\SourceClassTest::tearDownAfterClass
Exception in PhpUnit_Depends_Order\Test\SourceClassTest::tearDownAfterClass
One of the tests were skipped. Something wrong with the order of executed tests.

/infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/tests/SourceClassTest.php:27

ERRORS!
Tests: 2, Assertions: 0, Errors: 1, Failures: 1.

Errors mutants:
===============

Not Covered mutants:
====================

This output is no longer treated as "error" in infection.

The exit code is 2 on these lines:

infection/src/Process/MutantProcess.php

Lines 117 to 119 in f4a72a8

	if (!\in_array($this->getProcess()->getExitCode(), self::NOT_FATAL_ERROR_CODES, true)) {
	return self::CODE_ERROR;
	}

and condition is falsy.

But for PHPUnit < 8 exit code was 255.

Originally posted by @maks-rafalko in #738 (comment)

---

So, what was an error in PHPUnit <8, now is just a killed Mutant. IMO, we should update the code and support both cases (<8, 8+) to correctly "understand" (fatal) errors

[maks-rafalko]

Found that the new behaviour (returning exit code 2 instead of 255) is reproduced starting from PHPUnit 8.0.6.

Here is a diff sebastianbergmann/phpunit@8.0.5...8.0.6, but at this moment I have no idea what change causes this issue.

[sanmai]

sebastianbergmann/phpunit#3564 seems like the closest match. And that's exactly what we did:

infection/tests/Fixtures/e2e/PhpUnit_Depends_Order/tests/SourceClassTest.php

Lines 22 to 29 in 050cff2

	public static function tearDownAfterClass(): void
	{
	parent::tearDownAfterClass();
	if (self::$counter < 4) {
	throw new \Exception('One of the tests were skipped. Something wrong with the order of executed tests.');
	}
	}

I guess this is by design: failing with a fatal error is never appropriate. Otherwise put, it was a bug when PHPUnit exited with 255.

[sanmai]

Since this was a bug in PHPUnit, do you think there's anything we should do about it?

[maks-rafalko]

I think we should detect (fatal) errors not from the returned exit code, but by parsing PHPUnit process output.

Example:

No errors

There was 1 failure:

1) Infection\Tests\Process\MutantProcessTest::test_it_handles_not_covered_mutant
Failed asserting that 4 is identical to false.

Errors (syntax error):

There was 1 error:

1) Infection\Tests\Process\MutantProcessTest::test_it_handles_not_covered_mutant
ParseError: syntax error, unexpected '->' (T_OBJECT_OPERATOR)

Errors (exception):

There was 1 error:

1) Infection\Tests\Process\MutantProcessTest::test_it_handles_killed_mutant
Exception:

/infection/src/Process/MutantProcess.php:109

[maks-rafalko]

Ok, nerermind. This new behaviour is related to tearDownAfterClass() only, as I see.

Infection still understands errors and displays E where needed: https://travis-ci.org/infection/infection/jobs/558502375#L880