-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Output buffer - fix 5851 #5852
base: main
Are you sure you want to change the base?
Output buffer - fix 5851 #5852
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 @@ | |
private ?string $outputExpectedString = null; | ||
private bool $outputBufferingActive = false; | ||
private int $outputBufferingLevel; | ||
private ?int $outputBufferingFlushed = null; | ||
private bool $outputRetrievedForAssertion = false; | ||
private bool $doesNotPerformAssertions = false; | ||
|
||
|
@@ -452,7 +456,7 @@ | |
* | ||
* @internal This method is not covered by the backward compatibility promise for PHPUnit | ||
*/ | ||
final public function runBare(): void | ||
Check warning on line 459 in src/Framework/TestCase.php
|
||
{ | ||
$emitter = Event\Facade::emitter(); | ||
|
||
|
@@ -557,12 +561,14 @@ | |
|
||
$outputBufferingStopped = false; | ||
|
||
if (!isset($e) && | ||
Check warning on line 564 in src/Framework/TestCase.php
|
||
$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 @@ | |
$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; | ||
Check warning on line 1829 in src/Framework/TestCase.php
|
||
|
||
if (($phase & PHP_OUTPUT_HANDLER_FINAL) === PHP_OUTPUT_HANDLER_FINAL) { | ||
Check warning on line 1831 in src/Framework/TestCase.php
|
||
// 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']); | ||
Check warning on line 1850 in src/Framework/TestCase.php
|
||
|
||
$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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this escalation from risky test to test failure intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I didn't plan that initially, but as I created the PR, I realized that more likely than not these I did keep "risky" in some cases (as you can see) though, were the behavior is usually not much different than if there were no ob issue at all. Due to the nature of the buffer, the resulting logic/code is unfortunately a bit ugly, so feedback is very welcome. |
||
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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
--TEST-- | ||
https://github.com/sebastianbergmann/phpunit/pull/5861 | ||
--FILE-- | ||
<?php declare(strict_types=1); | ||
$_SERVER['argv'][] = '--do-not-cache-result'; | ||
$_SERVER['argv'][] = '--no-configuration'; | ||
$_SERVER['argv'][] = __DIR__ . '/5851/Issue5851Test.php'; | ||
|
||
require_once __DIR__ . '/../../bootstrap.php'; | ||
(new PHPUnit\TextUI\Application)->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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, it is a bugfix, so it must target the lowest supported and problematic branch.
Also, it seems this PR fixes several ob issues. I would like to see a separate commit for each issue ("Fix ob is not cleanable", ...) so each can be reviewed individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give me a quick hint, since this is my first PR for phpunit - which branch would that be? Rebasing/Rebranching from that and implementing it on that branch is sufficient or is there anything else?
Practically this isn't possible except for a single case (the error added for ob_flush/ob_clean), since all changes are needed for any of these cases to work correctly. Separating it would make it more confusing than it already is - due to the nature of output buffering.
In case you insist, could you please tell me the exact commits/how you want it separated, then I can see if it's possible or tell you exactly why it's not possible.