Skip to content

Commit df28ce6

Browse files
kkmuffmesebastianbergmann
kkmuffme
authored and
sebastianbergmann
committed
Fix 5851
1 parent 4899544 commit df28ce6

File tree

4 files changed

+473
-25
lines changed

4 files changed

+473
-25
lines changed

‎src/Framework/TestCase.php

Lines changed: 213 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,13 @@
3232
use function is_string;
3333
use function libxml_clear_errors;
3434
use function method_exists;
35+
use function ob_clean;
3536
use function ob_end_clean;
36-
use function ob_get_clean;
37+
use function ob_end_flush;
38+
use function ob_flush;
3739
use function ob_get_contents;
3840
use function ob_get_level;
41+
use function ob_get_status;
3942
use function ob_start;
4043
use function preg_match;
4144
use function restore_error_handler;
@@ -173,6 +176,7 @@ abstract class TestCase extends Assert implements Reorderable, SelfDescribing, T
173176
private ?string $outputExpectedString = null;
174177
private bool $outputBufferingActive = false;
175178
private int $outputBufferingLevel;
179+
private ?int $outputBufferingFlushed = null;
176180
private bool $outputRetrievedForAssertion = false;
177181
private bool $doesNotPerformAssertions = false;
178182

@@ -544,11 +548,13 @@ final public function runBare(): void
544548
$outputBufferingStopped = false;
545549

546550
if (!isset($e) &&
547-
$this->hasExpectationOnOutput() &&
548-
$this->stopOutputBuffering()) {
551+
$this->hasExpectationOnOutput()) {
552+
// if it fails now, we shouldn't try again later either
549553
$outputBufferingStopped = true;
550554

551-
$this->performAssertionsOnOutput();
555+
if ($this->stopOutputBuffering()) {
556+
$this->performAssertionsOnOutput();
557+
}
552558
}
553559

554560
if ($this->status->isSuccess()) {
@@ -1434,43 +1440,235 @@ private function markSkippedForMissingDependency(ExecutionOrderDependency $depen
14341440
$this->status = TestStatus::skipped($message);
14351441
}
14361442

1443+
private function outputBufferingCallback(string $output, int $phase): string
1444+
{
1445+
// assign it here to not get output from uncleanable children at the end
1446+
// as well as to ensure we have all output available to check
1447+
// if any children buffers have a low chunk size and already returned data
1448+
// or ob_flush was called
1449+
if ($this->outputBufferingActive) {
1450+
$this->output .= $output;
1451+
1452+
if (($phase & PHP_OUTPUT_HANDLER_FINAL) === PHP_OUTPUT_HANDLER_FINAL) {
1453+
// don't handle, since we report an error already if our handler is missing
1454+
// since it's inconsistent here with ob_end_flush/ob_end_clean
1455+
return '';
1456+
}
1457+
1458+
if (($phase & PHP_OUTPUT_HANDLER_CLEAN) === PHP_OUTPUT_HANDLER_CLEAN) {
1459+
$this->outputBufferingFlushed = PHP_OUTPUT_HANDLER_CLEAN;
1460+
} elseif (($phase & PHP_OUTPUT_HANDLER_FLUSH) === PHP_OUTPUT_HANDLER_FLUSH) {
1461+
$this->outputBufferingFlushed = PHP_OUTPUT_HANDLER_FLUSH;
1462+
}
1463+
}
1464+
1465+
return '';
1466+
}
1467+
1468+
// private to ensure it cannot be restarted after ending it from outside this class
14371469
private function startOutputBuffering(): void
14381470
{
1439-
ob_start();
1471+
ob_start([$this, 'outputBufferingCallback']);
14401472

14411473
$this->outputBufferingActive = true;
14421474
$this->outputBufferingLevel = ob_get_level();
14431475
}
14441476

1477+
/**
1478+
* @throws Exception
1479+
* @throws NoPreviousThrowableException
1480+
*/
14451481
private function stopOutputBuffering(): bool
14461482
{
1447-
$bufferingLevel = ob_get_level();
1483+
$bufferingLevel = ob_get_level();
1484+
$bufferingStatus = ob_get_status();
1485+
$bufferingCallbackName = $bufferingStatus['name'] ?? '';
1486+
$expectedBufferingCallable = static::class . '::outputBufferingCallback';
1487+
1488+
if ($bufferingLevel !== $this->outputBufferingLevel ||
1489+
($this->outputBufferingActive && $bufferingCallbackName !== $expectedBufferingCallable) ||
1490+
$this->outputBufferingFlushed !== null) {
1491+
$asFailure = true;
14481492

1449-
if ($bufferingLevel !== $this->outputBufferingLevel) {
14501493
if ($bufferingLevel > $this->outputBufferingLevel) {
14511494
$message = 'Test code or tested code did not close its own output buffers';
1452-
} else {
1495+
} elseif ($bufferingLevel < $this->outputBufferingLevel) {
14531496
$message = 'Test code or tested code closed output buffers other than its own';
1497+
} elseif ($this->outputBufferingActive && $bufferingCallbackName !== $expectedBufferingCallable) {
1498+
$message = 'Test code or tested code first closed output buffers other than its own and later started output buffers it did not close';
1499+
} elseif ($this->outputBufferingFlushed !== null) {
1500+
// if we weren't in phpunit this would lead to a PHP notice
1501+
$message = 'Test code or tested code flushed or cleaned global output buffers other than its own';
1502+
} else {
1503+
$this->outputBufferingLevel = ob_get_level();
1504+
1505+
return true;
1506+
}
1507+
1508+
$hasExpectedCallable = false;
1509+
1510+
if ($this->outputBufferingActive) {
1511+
$fullObStatus = ob_get_status(true);
1512+
$bufferingCallbackNameLevel = $fullObStatus[$this->outputBufferingLevel - 1]['name'] ?? '';
1513+
$bufferingCallbackSizeUsed = $fullObStatus[$this->outputBufferingLevel - 1]['buffer_used'] ?? PHP_INT_MAX;
1514+
1515+
if ($bufferingCallbackNameLevel === $expectedBufferingCallable) {
1516+
$hasExpectedCallable = true;
1517+
1518+
foreach ($fullObStatus as $index => $obStatus) {
1519+
if ($index < $this->outputBufferingLevel) {
1520+
continue;
1521+
}
1522+
1523+
if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_REMOVABLE) === PHP_OUTPUT_HANDLER_REMOVABLE) {
1524+
continue;
1525+
}
1526+
1527+
if (!$this->inIsolation && !$this->shouldRunInSeparateProcess()) {
1528+
$message = 'Test code contains a non-removable output buffer - run test in separate process to avoid side-effects';
1529+
$hasExpectedCallable = false;
1530+
1531+
break;
1532+
}
1533+
1534+
// allow non-removable handler 1 level deeper than our handler to allow unit tests for non-removable handlers
1535+
// 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
1536+
if ($index === $this->outputBufferingLevel && $bufferingCallbackSizeUsed === 0) {
1537+
continue;
1538+
}
1539+
1540+
if ($index === $this->outputBufferingLevel) {
1541+
$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';
1542+
} else {
1543+
// we cannot get the data from the handlers between our handler and the topmost non-removable handler
1544+
$message = 'Tests with multiple output buffers where any, except the first, are non-removable are not supported';
1545+
}
1546+
1547+
$hasExpectedCallable = false;
1548+
1549+
break;
1550+
}
1551+
1552+
if ($hasExpectedCallable && $this->outputBufferingFlushed === null) {
1553+
$asFailure = false;
1554+
}
1555+
} else {
1556+
// the original buffer doesn't exist anymore at that level, which means it was closed
1557+
$message = 'Test code or tested code first closed output buffers other than its own and later started output buffers it did not close';
1558+
}
1559+
} else {
1560+
$asFailure = false;
14541561
}
14551562

14561563
while (ob_get_level() >= $this->outputBufferingLevel) {
1457-
ob_end_clean();
1564+
$obStatus = ob_get_status();
1565+
1566+
if ($obStatus === []) {
1567+
break;
1568+
}
1569+
1570+
// 'level' is off by 1 because 0-indexed
1571+
if ($hasExpectedCallable && $obStatus['name'] === $expectedBufferingCallable && $obStatus['level'] + 1 === $this->outputBufferingLevel) {
1572+
// our own handler
1573+
ob_end_clean();
1574+
1575+
continue;
1576+
}
1577+
1578+
if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_REMOVABLE) === PHP_OUTPUT_HANDLER_REMOVABLE) {
1579+
// bubble it up
1580+
ob_end_flush();
1581+
1582+
continue;
1583+
}
1584+
1585+
if ($hasExpectedCallable && $obStatus['level'] === $this->outputBufferingLevel) {
1586+
$fullObStatus = ob_get_status(true);
1587+
$bufferingCallbackSizeUsed = $fullObStatus[$this->outputBufferingLevel - 1]['buffer_used'] ?? PHP_INT_MAX;
1588+
1589+
// we are 1 level deeper than our buffer
1590+
// check again to be sure, as we cannot retrieve what's in the buffer
1591+
if ($bufferingCallbackSizeUsed !== 0) {
1592+
$hasExpectedCallable = false;
1593+
$asFailure = true;
1594+
$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';
1595+
} else {
1596+
// assign it since we cannot trigger our callback
1597+
// this is the reason why it's risky even then, since the ob callback of the non-removable buffer isn't called
1598+
// which could modify the output
1599+
$this->output .= (string) ob_get_contents();
1600+
1601+
// if we have the default output handler which doesn't modify output
1602+
// this isn't even risky
1603+
if ($obStatus['name'] === 'default output handler' && $this->outputBufferingFlushed === null) {
1604+
$message = null;
1605+
} elseif ($this->outputBufferingFlushed === null) {
1606+
$message = 'Non-removable output handler callback was not called, which could alter output';
1607+
} else {
1608+
$asFailure = true;
1609+
}
1610+
}
1611+
} elseif (($obStatus['flags'] & PHP_OUTPUT_HANDLER_FLUSHABLE) === PHP_OUTPUT_HANDLER_FLUSHABLE) {
1612+
// bubble it up
1613+
ob_flush();
1614+
}
1615+
1616+
if (($obStatus['flags'] & PHP_OUTPUT_HANDLER_CLEANABLE) === PHP_OUTPUT_HANDLER_CLEANABLE) {
1617+
// make sure it's empty for subsequent runs to reduce unrelated errors
1618+
ob_clean();
1619+
}
1620+
1621+
// can't end any parents either
1622+
break;
14581623
}
14591624

1460-
Event\Facade::emitter()->testConsideredRisky(
1625+
// reset it to stop adding more output
1626+
$this->outputBufferingActive = false;
1627+
$this->outputBufferingFlushed = null;
1628+
$this->outputBufferingLevel = ob_get_level();
1629+
1630+
if ($message === null) {
1631+
return true;
1632+
}
1633+
1634+
if (!$asFailure) {
1635+
Event\Facade::emitter()->testConsideredRisky(
1636+
$this->valueObjectForEvents(),
1637+
$message,
1638+
);
1639+
1640+
$this->status = TestStatus::risky($message);
1641+
1642+
return true;
1643+
}
1644+
1645+
// it's impossible to tell if there were any PHP errors or failed assertions
1646+
$this->status = TestStatus::failure($message);
1647+
1648+
Event\Facade::emitter()->testFailed(
14611649
$this->valueObjectForEvents(),
1462-
$message,
1650+
Event\Code\ThrowableBuilder::from(new Exception($message)),
1651+
null,
14631652
);
14641653

1465-
$this->status = TestStatus::risky($message);
1654+
if ($this->numberOfAssertionsPerformed() === 0 &&
1655+
$this->hasExpectationOnOutput()) {
1656+
// no error that no assertions were performed
1657+
$this->addToAssertionCount(1);
1658+
}
14661659

14671660
return false;
14681661
}
14691662

1470-
$this->output = ob_get_clean();
1663+
if (!$this->outputBufferingActive) {
1664+
return true;
1665+
}
14711666

1472-
$this->outputBufferingActive = false;
1473-
$this->outputBufferingLevel = ob_get_level();
1667+
ob_end_clean();
1668+
1669+
$this->outputBufferingActive = false;
1670+
$this->outputBufferingFlushed = null;
1671+
$this->outputBufferingLevel = ob_get_level();
14741672

14751673
return true;
14761674
}

‎tests/end-to-end/regression/5342.phpt

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,15 @@ F 1 / 1 (100%)
1717

1818
Time: %s, Memory: %s
1919

20-
There was 1 failure:
20+
There were 2 failures:
2121

2222
1) PHPUnit\TestFixture\Issue5342Test::testFailure
2323
Failed asserting that false is true.
2424

2525
%sIssue5342Test.php:%i
2626

27-
--
28-
29-
There was 1 risky test:
30-
31-
1) PHPUnit\TestFixture\Issue5342Test::testFailure
32-
Test code or tested code closed output buffers other than its own
33-
34-
%sIssue5342Test.php:%i
27+
2) PHPUnit\TestFixture\Issue5342Test::testFailure
28+
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
3529

3630
FAILURES!
37-
Tests: 1, Assertions: 1, Failures: 1, Risky: 1.
31+
Tests: 1, Assertions: 1, Failures: 2.

‎tests/end-to-end/regression/5851.phpt

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
--TEST--
2+
https://github.com/sebastianbergmann/phpunit/pull/5861
3+
--FILE--
4+
<?php declare(strict_types=1);
5+
$_SERVER['argv'][] = '--do-not-cache-result';
6+
$_SERVER['argv'][] = '--no-configuration';
7+
$_SERVER['argv'][] = __DIR__ . '/5851/Issue5851Test.php';
8+
9+
require_once __DIR__ . '/../../bootstrap.php';
10+
(new PHPUnit\TextUI\Application)->run($_SERVER['argv']);
11+
--EXPECTF--
12+
PHPUnit %s by Sebastian Bergmann and contributors.
13+
14+
Runtime: %s
15+
16+
FFFIllegaly hide thisFFSneakyFNaughtySafeFFRRFRF. 14 / 14 (100%)
17+
18+
Time: %s, Memory: %s
19+
20+
There were 10 failures:
21+
22+
1) PHPUnit\TestFixture\Issue5851Test::testInvalidFlushBuffer
23+
PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own
24+
25+
2) PHPUnit\TestFixture\Issue5851Test::testInvalidSilencedFlushBuffer
26+
PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own
27+
28+
3) PHPUnit\TestFixture\Issue5851Test::testInvalidFlushBufferEmpty
29+
PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own
30+
31+
4) PHPUnit\TestFixture\Issue5851Test::testInvalidCleanExternalBuffer
32+
PHPUnit\Framework\Exception: Test code or tested code flushed or cleaned global output buffers other than its own
33+
34+
5) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferNoOutput
35+
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
36+
37+
6) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferOutput
38+
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
39+
40+
7) PHPUnit\TestFixture\Issue5851Test::testRemovedAndAddedBufferExpectedOutput
41+
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
42+
43+
8) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored
44+
Failed asserting that two strings are identical.
45+
--- Expected
46+
+++ Actual
47+
@@ @@
48+
-''
49+
+'Do not ignore thisor this'
50+
51+
9) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBuffer
52+
PHPUnit\Framework\Exception: Test code contains a non-removable output buffer - run test in separate process to avoid side-effects
53+
54+
10) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferChunkSizeTooLow
55+
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
56+
57+
--
58+
59+
There were 4 risky tests:
60+
61+
1) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored
62+
Test code or tested code did not close its own output buffers
63+
64+
%s%eIssue5851Test.php:%i
65+
66+
2) PHPUnit\TestFixture\Issue5851Test::testNonClosedBufferShouldntBeIgnored2
67+
Test code or tested code did not close its own output buffers
68+
69+
%s%eIssue5851Test.php:%i
70+
71+
3) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferSeparateProcess
72+
Non-removable output handler callback was not called, which could alter output
73+
74+
%s%eIssue5851Test.php:%i
75+
76+
4) PHPUnit\TestFixture\Issue5851Test::testNonRemovableBufferSeparateProcessAgain
77+
Non-removable output handler callback was not called, which could alter output
78+
79+
%s%eIssue5851Test.php:%i
80+
81+
FAILURES!
82+
Tests: 14, Assertions: 14, Failures: 10, Risky: 4.

0 commit comments

Comments
 (0)