From 6e8e56aebdff6eee1f90266841be82e5978982f3 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sun, 2 Jun 2024 22:43:33 +0200 Subject: [PATCH] Fix 5851 --- src/Framework/TestCase.php | 228 ++++++++++++++++-- tests/end-to-end/regression/5342.phpt | 14 +- tests/end-to-end/regression/5851.phpt | 82 +++++++ .../regression/5851/Issue5851Test.php | 174 +++++++++++++ 4 files changed, 473 insertions(+), 25 deletions(-) create mode 100644 tests/end-to-end/regression/5851.phpt create mode 100644 tests/end-to-end/regression/5851/Issue5851Test.php diff --git a/src/Framework/TestCase.php b/src/Framework/TestCase.php index cf1e781cbec..385eb135f11 100644 --- a/src/Framework/TestCase.php +++ b/src/Framework/TestCase.php @@ -42,10 +42,13 @@ use function is_string; use function libxml_clear_errors; use function method_exists; +use function ob_clean; use function ob_end_clean; -use function ob_get_clean; +use function ob_end_flush; +use function ob_flush; use function ob_get_contents; use function ob_get_level; +use function ob_get_status; use function ob_start; use function parse_url; use function pathinfo; @@ -191,6 +194,7 @@ abstract class TestCase extends Assert implements Reorderable, SelfDescribing, T private ?string $outputExpectedString = null; private bool $outputBufferingActive = false; private int $outputBufferingLevel; + private ?int $outputBufferingFlushed = null; private bool $outputRetrievedForAssertion = false; private bool $doesNotPerformAssertions = false; @@ -558,11 +562,13 @@ final public function runBare(): void $outputBufferingStopped = false; if (!isset($e) && - $this->hasExpectationOnOutput() && - $this->stopOutputBuffering()) { + $this->hasExpectationOnOutput()) { + // if it fails now, we shouldn't try again later either $outputBufferingStopped = true; - $this->performAssertionsOnOutput(); + if ($this->stopOutputBuffering()) { + $this->performAssertionsOnOutput(); + } } if ($this->status->isSuccess()) { @@ -1813,43 +1819,235 @@ private function markSkippedForMissingDependency(ExecutionOrderDependency $depen $this->status = TestStatus::skipped($message); } + private function outputBufferingCallback(string $output, int $phase): string + { + // assign it here to not get output from uncleanable children at the end + // as well as to ensure we have all output available to check + // if any children buffers have a low chunk size and already returned data + // or ob_flush was called + if ($this->outputBufferingActive) { + $this->output .= $output; + + if (($phase & PHP_OUTPUT_HANDLER_FINAL) === PHP_OUTPUT_HANDLER_FINAL) { + // don't handle, since we report an error already if our handler is missing + // since it's inconsistent here with ob_end_flush/ob_end_clean + return ''; + } + + if (($phase & PHP_OUTPUT_HANDLER_CLEAN) === PHP_OUTPUT_HANDLER_CLEAN) { + $this->outputBufferingFlushed = PHP_OUTPUT_HANDLER_CLEAN; + } elseif (($phase & PHP_OUTPUT_HANDLER_FLUSH) === PHP_OUTPUT_HANDLER_FLUSH) { + $this->outputBufferingFlushed = PHP_OUTPUT_HANDLER_FLUSH; + } + } + + return ''; + } + + // private to ensure it cannot be restarted after ending it from outside this class private function startOutputBuffering(): void { - ob_start(); + ob_start([$this, 'outputBufferingCallback']); $this->outputBufferingActive = true; $this->outputBufferingLevel = ob_get_level(); } + /** + * @throws Exception + * @throws NoPreviousThrowableException + */ private function stopOutputBuffering(): bool { - $bufferingLevel = ob_get_level(); + $bufferingLevel = ob_get_level(); + $bufferingStatus = ob_get_status(); + $bufferingCallbackName = $bufferingStatus['name'] ?? ''; + $expectedBufferingCallable = static::class . '::outputBufferingCallback'; + + if ($bufferingLevel !== $this->outputBufferingLevel || + ($this->outputBufferingActive && $bufferingCallbackName !== $expectedBufferingCallable) || + $this->outputBufferingFlushed !== null) { + $asFailure = true; - if ($bufferingLevel !== $this->outputBufferingLevel) { if ($bufferingLevel > $this->outputBufferingLevel) { $message = 'Test code or tested code did not close its own output buffers'; - } else { + } elseif ($bufferingLevel < $this->outputBufferingLevel) { $message = 'Test code or tested code closed output buffers other than its own'; + } elseif ($this->outputBufferingActive && $bufferingCallbackName !== $expectedBufferingCallable) { + $message = 'Test code or tested code first closed output buffers other than its own and later started output buffers it did not close'; + } elseif ($this->outputBufferingFlushed !== null) { + // if we weren't in phpunit this would lead to a PHP notice + $message = 'Test code or tested code flushed or cleaned global output buffers other than its own'; + } else { + $this->outputBufferingLevel = ob_get_level(); + + return true; + } + + $hasExpectedCallable = false; + + if ($this->outputBufferingActive) { + $fullObStatus = ob_get_status(true); + $bufferingCallbackNameLevel = $fullObStatus[$this->outputBufferingLevel - 1]['name'] ?? ''; + $bufferingCallbackSizeUsed = $fullObStatus[$this->outputBufferingLevel - 1]['buffer_used'] ?? PHP_INT_MAX; + + if ($bufferingCallbackNameLevel === $expectedBufferingCallable) { + $hasExpectedCallable = true; + + foreach ($fullObStatus as $index => $obStatus) { + if ($index < $this->outputBufferingLevel) { + continue; + } + + if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_REMOVABLE) === PHP_OUTPUT_HANDLER_REMOVABLE) { + continue; + } + + if (!$this->inIsolation && !$this->shouldRunInSeparateProcess()) { + $message = 'Test code contains a non-removable output buffer - run test in separate process to avoid side-effects'; + $hasExpectedCallable = false; + + break; + } + + // allow non-removable handler 1 level deeper than our handler to allow unit tests for non-removable handlers + // however only if our own handler is empty, as we cannot retrieve that from our handler if we are in a non-removable handler in a level deeper + if ($index === $this->outputBufferingLevel && $bufferingCallbackSizeUsed === 0) { + continue; + } + + if ($index === $this->outputBufferingLevel) { + $message = 'Tests with non-removable output buffer handlers must not call flush on them and the chunk size must be bigger than the expected output'; + } else { + // we cannot get the data from the handlers between our handler and the topmost non-removable handler + $message = 'Tests with multiple output buffers where any, except the first, are non-removable are not supported'; + } + + $hasExpectedCallable = false; + + break; + } + + if ($hasExpectedCallable && $this->outputBufferingFlushed === null) { + $asFailure = false; + } + } else { + // the original buffer doesn't exist anymore at that level, which means it was closed + $message = 'Test code or tested code first closed output buffers other than its own and later started output buffers it did not close'; + } + } else { + $asFailure = false; } while (ob_get_level() >= $this->outputBufferingLevel) { - ob_end_clean(); + $obStatus = ob_get_status(); + + if ($obStatus === []) { + break; + } + + // 'level' is off by 1 because 0-indexed + if ($hasExpectedCallable && $obStatus['name'] === $expectedBufferingCallable && $obStatus['level'] + 1 === $this->outputBufferingLevel) { + // our own handler + ob_end_clean(); + + continue; + } + + if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_REMOVABLE) === PHP_OUTPUT_HANDLER_REMOVABLE) { + // bubble it up + ob_end_flush(); + + continue; + } + + if ($hasExpectedCallable && $obStatus['level'] === $this->outputBufferingLevel) { + $fullObStatus = ob_get_status(true); + $bufferingCallbackSizeUsed = $fullObStatus[$this->outputBufferingLevel - 1]['buffer_used'] ?? PHP_INT_MAX; + + // we are 1 level deeper than our buffer + // check again to be sure, as we cannot retrieve what's in the buffer + if ($bufferingCallbackSizeUsed !== 0) { + $hasExpectedCallable = false; + $asFailure = true; + $message = 'Tests with non-removable output buffer handlers must not call flush on them and the chunk size must be bigger than the expected output'; + } else { + // assign it since we cannot trigger our callback + // this is the reason why it's risky even then, since the ob callback of the non-removable buffer isn't called + // which could modify the output + $this->output .= (string) ob_get_contents(); + + // if we have the default output handler which doesn't modify output + // this isn't even risky + if ($obStatus['name'] === 'default output handler' && $this->outputBufferingFlushed === null) { + $message = null; + } elseif ($this->outputBufferingFlushed === null) { + $message = 'Non-removable output handler callback was not called, which could alter output'; + } else { + $asFailure = true; + } + } + } elseif (($obStatus['flags'] & PHP_OUTPUT_HANDLER_FLUSHABLE) === PHP_OUTPUT_HANDLER_FLUSHABLE) { + // bubble it up + ob_flush(); + } + + if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_CLEANABLE) === PHP_OUTPUT_HANDLER_CLEANABLE) { + // make sure it's empty for subsequent runs to reduce unrelated errors + ob_clean(); + } + + // can't end any parents either + break; } - Event\Facade::emitter()->testConsideredRisky( + // reset it to stop adding more output + $this->outputBufferingActive = false; + $this->outputBufferingFlushed = null; + $this->outputBufferingLevel = ob_get_level(); + + if ($message === null) { + return true; + } + + if (!$asFailure) { + Event\Facade::emitter()->testConsideredRisky( + $this->valueObjectForEvents(), + $message, + ); + + $this->status = TestStatus::risky($message); + + return true; + } + + // it's impossible to tell if there were any PHP errors or failed assertions + $this->status = TestStatus::failure($message); + + Event\Facade::emitter()->testFailed( $this->valueObjectForEvents(), - $message, + Event\Code\ThrowableBuilder::from(new Exception($message)), + null, ); - $this->status = TestStatus::risky($message); + if ($this->numberOfAssertionsPerformed() === 0 && + $this->hasExpectationOnOutput()) { + // no error that no assertions were performed + $this->addToAssertionCount(1); + } return false; } - $this->output = ob_get_clean(); + if (!$this->outputBufferingActive) { + return true; + } - $this->outputBufferingActive = false; - $this->outputBufferingLevel = ob_get_level(); + ob_end_clean(); + + $this->outputBufferingActive = false; + $this->outputBufferingFlushed = null; + $this->outputBufferingLevel = ob_get_level(); return true; } diff --git a/tests/end-to-end/regression/5342.phpt b/tests/end-to-end/regression/5342.phpt index e8e6ee34b70..245575b7173 100644 --- a/tests/end-to-end/regression/5342.phpt +++ b/tests/end-to-end/regression/5342.phpt @@ -17,21 +17,15 @@ F 1 / 1 (100%) Time: %s, Memory: %s -There was 1 failure: +There were 2 failures: 1) PHPUnit\TestFixture\Issue5342Test::testFailure Failed asserting that false is true. %sIssue5342Test.php:%i --- - -There was 1 risky test: - -1) PHPUnit\TestFixture\Issue5342Test::testFailure -Test code or tested code closed output buffers other than its own - -%sIssue5342Test.php:%i +2) PHPUnit\TestFixture\Issue5342Test::testFailure +PHPUnit\Framework\Exception: Test code or tested code first closed output buffers other than its own and later started output buffers it did not close FAILURES! -Tests: 1, Assertions: 1, Failures: 1, Risky: 1. +Tests: 1, Assertions: 1, Failures: 2. diff --git a/tests/end-to-end/regression/5851.phpt b/tests/end-to-end/regression/5851.phpt new file mode 100644 index 00000000000..87e9fe5759b --- /dev/null +++ b/tests/end-to-end/regression/5851.phpt @@ -0,0 +1,82 @@ +--TEST-- +https://github.com/sebastianbergmann/phpunit/pull/5861 +--FILE-- +run($_SERVER['argv']); +--EXPECTF-- +PHPUnit %s by Sebastian Bergmann and contributors. + +Runtime: %s + +FFFIllegaly hide thisFFSneakyFNaughtySafeFFRRFRF. 14 / 14 (100%) + +Time: %s, Memory: %s + +There were 10 failures: + +1) PHPUnit\TestFixture\Issue5851Test::testInvalidFlushBuffer +PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own + +2) PHPUnit\TestFixture\Issue5851Test::testInvalidSilencedFlushBuffer +PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own + +3) PHPUnit\TestFixture\Issue5851Test::testInvalidFlushBufferEmpty +PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own + +4) PHPUnit\TestFixture\Issue5851Test::testInvalidCleanExternalBuffer +PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own + +5) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferNoOutput +PHPUnit\Framework\Exception: Test code or tested code first closed output buffers other than its own and later started output buffers it did not close + +6) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferOutput +PHPUnit\Framework\Exception: Test code or tested code first closed output buffers other than its own and later started output buffers it did not close + +7) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferExpectedOutput +PHPUnit\Framework\Exception: Test code or tested code first closed output buffers other than its own and later started output buffers it did not close + +8) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored +Failed asserting that two strings are identical. +--- Expected ++++ Actual +@@ @@ +-'' ++'Do not ignore thisor this' + +9) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBuffer +PHPUnit\Framework\Exception: Test code contains a non-removable output buffer - run test in separate process to avoid side-effects + +10) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferChunkSizeTooLow +PHPUnit\Framework\Exception: Tests with non-removable output buffer handlers must not call flush on them and the chunk size must be bigger than the expected output + +-- + +There were 4 risky tests: + +1) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored +Test code or tested code did not close its own output buffers + +%s%eIssue5851Test.php:%i + +2) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored2 +Test code or tested code did not close its own output buffers + +%s%eIssue5851Test.php:%i + +3) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferSeparateProcess +Non-removable output handler callback was not called, which could alter output + +%s%eIssue5851Test.php:%i + +4) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferSeparateProcessAgain +Non-removable output handler callback was not called, which could alter output + +%s%eIssue5851Test.php:%i + +FAILURES! +Tests: 14, Assertions: 14, Failures: 10, Risky: 4. diff --git a/tests/end-to-end/regression/5851/Issue5851Test.php b/tests/end-to-end/regression/5851/Issue5851Test.php new file mode 100644 index 00000000000..0b5170e0ea4 --- /dev/null +++ b/tests/end-to-end/regression/5851/Issue5851Test.php @@ -0,0 +1,174 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ +namespace PHPUnit\TestFixture; + +use function ob_clean; +use function ob_end_clean; +use function ob_flush; +use function ob_start; +use PHPUnit\Framework\Attributes\RunInSeparateProcess; +use PHPUnit\Framework\TestCase; + +class Issue5851Test extends TestCase +{ + public function testInvalidFlushBuffer(): void + { + $this->expectOutputString('hello'); + + print 'hello'; + + ob_flush(); + } + + public function testInvalidSilencedFlushBuffer(): void + { + $this->expectOutputString('hello'); + + print 'hello'; + + @ob_flush(); + } + + public function testInvalidFlushBufferEmpty(): void + { + $this->expectOutputString(''); + + ob_flush(); + } + + public function testInvalidCleanExternalBuffer(): void + { + print 'Illegaly hide this'; + + $this->assertTrue(ob_clean()); + } + + public function testRemovedAndAddedBufferNoOutput(): void + { + $this->expectOutputString(''); + + ob_end_clean(); + + ob_start(); + } + + public function testRemovedAndAddedBufferOutput(): void + { + $this->expectOutputString(''); + + ob_end_clean(); + + print 'Sneaky'; + + ob_start(); + } + + public function testRemovedAndAddedBufferExpectedOutput(): void + { + $this->expectOutputString('Safe'); + + ob_end_clean(); + + print 'Naughty'; + + ob_start(); + + print 'Safe'; + } + + public function testNonClosedBufferShouldntBeIgnored(): void + { + $this->expectOutputString(''); + + ob_start(); + ob_start(); + ob_start(); + print 'Do not ignore this'; + ob_start(); + print 'or this'; + } + + public function testNonClosedBufferShouldntBeIgnored2(): void + { + $this->expectOutputString('Do not ignore thisor this'); + + ob_start(); + ob_start(); + ob_start(); + print 'Do not ignore this'; + ob_start(); + print 'or this'; + } + + #[RunInSeparateProcess] + public function testNonRemovableBufferSeparateProcess(): void + { + $this->expectOutputString('helloworldbye'); + + $this->runNonRemovableBuffer([$this, 'bufferCallbackA'], 0, 0); + } + + public function testNonRemovableBuffer(): void + { + $this->expectOutputString('helloworldbye'); + + $this->runNonRemovableBuffer([$this, 'bufferCallbackB'], 0, 0); + } + + /** + * check that the previous buffer that hasn't been ended won't break this. + */ + #[RunInSeparateProcess] + public function testNonRemovableBufferSeparateProcessAgain(): void + { + $this->expectOutputString('helloworldbye'); + + $this->runNonRemovableBuffer([$this, 'bufferCallbackC'], 0, 0); + } + + #[RunInSeparateProcess] + public function testNonRemovableBufferChunkSizeTooLow(): void + { + $this->expectOutputString('helloworldbye'); + + $this->runNonRemovableBuffer([$this, 'bufferCallbackC'], 1, 0); + } + + #[RunInSeparateProcess] + public function testEmptyNonRemovableBufferSeparateProcess(): void + { + $this->expectOutputString(''); + + ob_start(null, 0, 0); + } + + public function runNonRemovableBuffer(?callable $callable, int $chunk = 0, int $flags = 0): void + { + ob_start($callable, $chunk, $flags); + print 'hello'; + print 'world'; + print 'bye'; + } + + private function bufferCallbackA(string $output): string + { + return $output; + } + + private function bufferCallbackB(string $output): string + { + return $output; + } + + private function bufferCallbackC(string $output): string + { + return $output; + } +}