From 30db7f9f3e2b6aa654408dd37587637479ad8b99 Mon Sep 17 00:00:00 2001
From: Maks Rafalko <b0rn@list.ru>
Date: Wed, 22 Dec 2021 09:09:47 +0300
Subject: [PATCH] Mark Mutant as killed if Test Framework returns non-zero exit
 code (#1621)

PHPUnit can return "Tests passed! OK (10 tests, 32 assertions)" output, however return code will be non-zero.

This happens when, for example, `symfony/phpunit-bridge` is used, and it detects outstanding deprecations which fails PHPUnit execution, while all the tests are passing

See https://github.com/infection/infection/issues/1620#issuecomment-999073728

Now, when Test Framework exit code is > 100, Mutant will be treated as `E`rrored (retained behavior).

When Test Framework exit code is non-zero, Mutant will be treated as Killed (new behavior), even if from output's point of view tests pass.
---
 src/Mutant/MutantExecutionResultFactory.php   |  2 +-
 .../MutantExecutionResultFactoryTest.php      | 96 ++++++++++++++++++-
 2 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/src/Mutant/MutantExecutionResultFactory.php b/src/Mutant/MutantExecutionResultFactory.php
index c17731dd5..cc47a010f 100644
--- a/src/Mutant/MutantExecutionResultFactory.php
+++ b/src/Mutant/MutantExecutionResultFactory.php
@@ -106,7 +106,7 @@ private function retrieveDetectionStatus(MutantProcess $mutantProcess): string
 
         $output = $this->retrieveProcessOutput($process);
 
-        if ($this->testFrameworkAdapter->testsPass($output)) {
+        if ($process->getExitCode() === 0 && $this->testFrameworkAdapter->testsPass($output)) {
             return DetectionStatus::ESCAPED;
         }
 
diff --git a/tests/phpunit/Mutant/MutantExecutionResultFactoryTest.php b/tests/phpunit/Mutant/MutantExecutionResultFactoryTest.php
index 36c15f478..c3db83e44 100644
--- a/tests/phpunit/Mutant/MutantExecutionResultFactoryTest.php
+++ b/tests/phpunit/Mutant/MutantExecutionResultFactoryTest.php
@@ -316,7 +316,7 @@ public function test_it_can_crate_a_result_from_an_escaped_mutant_process(): voi
             ->willReturn('Tests passed!')
         ;
         $processMock
-            ->expects($this->once())
+            ->expects($this->exactly(2))
             ->method('getExitCode')
             ->willReturn(0)
         ;
@@ -399,7 +399,7 @@ public function test_it_can_crate_a_result_from_a_killed_mutant_process(): void
             ->willReturn('Tests failed!')
         ;
         $processMock
-            ->expects($this->once())
+            ->expects($this->exactly(2))
             ->method('getExitCode')
             ->willReturn(0)
         ;
@@ -463,4 +463,96 @@ public function test_it_can_crate_a_result_from_a_killed_mutant_process(): void
             $originalStartingLine
         );
     }
+
+    /**
+     * PHPUnit can return "Tests passed! OK (10 tests, 32 assertions)" output, however
+     * return code will be non-zero.
+     *
+     * This happens when, for example, symfony/phpunit-bridge is used, and it detects
+     * outstanding deprecations which fails PHPUnit execution, while all the tests are passing
+     *
+     * See https://github.com/infection/infection/issues/1620#issuecomment-999073728
+     */
+    public function test_it_marks_mutant_as_killed_when_tests_pass_from_output_but_exit_code_is_non_zero(): void
+    {
+        $processMock = $this->createMock(Process::class);
+        $processMock
+            ->method('getCommandLine')
+            ->willReturn(
+                $processCommandLine = 'bin/phpunit --configuration infection-tmp-phpunit.xml --filter "tests/Acme/FooTest.php"'
+            )
+        ;
+        $processMock
+            ->method('isTerminated')
+            ->willReturn(true)
+        ;
+        $processMock
+            ->method('getOutput')
+            ->willReturn('Tests passed! OK (10 tests, 32 assertions)')
+        ;
+        $processMock
+            ->expects($this->exactly(2))
+            ->method('getExitCode')
+            ->willReturn(1) // PHPUnit says tests passed, but return code is non-zero
+        ;
+
+        $this->testFrameworkAdapterMock
+            ->expects($this->never())
+            ->method('testsPass')
+            ->with('Tests passed! OK (10 tests, 32 assertions)')
+            ->willReturn(true)
+        ;
+
+        $mutantProcess = new MutantProcess(
+            $processMock,
+            MutantBuilder::build(
+                '/path/to/mutant',
+                new Mutation(
+                    $originalFilePath = 'path/to/Foo.php',
+                    [],
+                    $mutatorName = MutatorName::getName(For_::class),
+                    [
+                        'startLine' => $originalStartingLine = 10,
+                        'endLine' => 15,
+                        'startTokenPos' => 0,
+                        'endTokenPos' => 8,
+                        'startFilePos' => 2,
+                        'endFilePos' => 4,
+                    ],
+                    'Unknown',
+                    MutatedNode::wrap(new Nop()),
+                    0,
+                    [
+                        new TestLocation(
+                            'FooTest::test_it_can_instantiate',
+                            '/path/to/acme/FooTest.php',
+                            0.01
+                        ),
+                    ]
+                ),
+                'killed#0',
+                $mutantDiff = <<<'DIFF'
+--- Original
++++ New
+@@ @@
+
+- echo 'original';
++ echo 'killed#0';
+
+DIFF,
+                '<?php $a = 1;'
+            )
+        );
+
+        $this->assertResultStateIs(
+            $this->resultFactory->createFromProcess($mutantProcess),
+            $processCommandLine,
+            'Tests passed! OK (10 tests, 32 assertions)',
+            DetectionStatus::KILLED,
+            $mutantDiff,
+            $mutatorName,
+            $originalFilePath,
+            $originalStartingLine
+        );
+    }
 }