bug: Improve handling exceptions from unsupported PHPUnit versions#1521
bug: Improve handling exceptions from unsupported PHPUnit versions#1521acoulton merged 2 commits intoBehat:masterfrom
Conversation
|
As the github action logs will eventually be removed, just to record that the previous output (for the entire behat command) was: With a theoretically-supported PHPUnit that causes errors during stringificationWith unsupported PHPUnit[... stack frames 7 to 31 removed for simplicity...] Note that in the second case the ~36 line exception and stack trace is rendered multiple times - once for each failing scenario. |
carlos-granados
left a comment
carlos-granados
left a comment
There was a problem hiding this comment.
Had to do a deep read to get to the bottom of this, looks like a good solution overall, I just added a couple of small comments
src/Behat/Testwork/Exception/Stringer/PHPUnitExceptionStringer.php
Outdated
Show resolved
Hide resolved
Attempting to render PHPUnit assertion failures is brittle, because a) we (intentionally) don't impose any PHPUnit version constraints in composer.json and b) PHPUnit's code for rendering exceptions is not covered by their BC promise. This update to the PHPUnitExceptionStringer ensures that we always render something useful - prioritising communicating that an assertion has failed - even if we don't correctly support the PHPUnit version in use.
acoulton
force-pushed
the
bug-phpunit-error-handling
branch
from
2b6c86f to
2c7d664
Compare
carlos-granados
left a comment
carlos-granados
left a comment
There was a problem hiding this comment.
Looks all good. Would still like @uuf6429 to check this as well
|
Looks very good overall @acoulton! |
| static::assertClassNotLoaded(\PHPUnit\Framework\TestFailure::class); | ||
|
|
||
| if (PHP_VERSION_ID < 80400) { | ||
| // Trigger loading array_find from symfony/polyfill-php84 before our autoloader tries to use it |
There was a problem hiding this comment.
there is no function autoloading in PHP, so this is totally useless. The function polyfills are loaded eagerly at the time you load the vendor/autoload.php file
There was a problem hiding this comment.
@stof the functions are autoloaded but until you call them they don't try to load the Php84 class that the polyfill actually uses. I know because without this the autoloader failed with an error due to not autoloading the symfony class.
There was a problem hiding this comment.
@stof I was on the move earlier, here is how symfony/polyfill implements array_find:
if (!function_exists('array_find')) {
function array_find(array $array, callable $callback) { return p\Php84::array_find($array, $callback); }
}
As you can see, if this is the first Php84 polyfill that has been called, it will trigger an autoload of the Php84 class. Not normally an issue, but if you're using array_find inside an autoloader that can be a problem as indeed it did here.
FWIW, it may be just a language thing, or that I am tired after a long week, but I read your comment as quite rude. From my perspective it didn't feel great to have you engage with a PR that has involved a fair bit of effort and thought, and that fixes a longstanding issue, and your only comment is to describe a tiny part of it as "totally useless".
I flag this less for my own sake, more that if we want to get behat moving again I hope we can all try to make contributors feel welcome and valued.
4 participants
This is an alternative fix for #1421, based partly on the useful work done by and discussion with @uuf6429 in #1426.
Attempting to render PHPUnit assertion failures is brittle, because:
a) we (intentionally) don't impose any PHPUnit version constraints in composer.json and
b) PHPUnit's code for rendering exceptions is not covered by their BC promise.
I've added tests that cover both the absence of PHPUnit classes, and the presence of a PHPUnit class that works differently to how we expect, causing an error. These demonstrate the current total failure and early termination of Behat in both these scenarios (and show that throwing our own exception here will behave the same, just with a different message).
The second commit updates the PHPUnitExceptionStringer to ensure that we always render something useful for any past or future PHPUnit version. This prioritises communicating that an assertion has failed and in many cases will still have as much detail as usual (see e.g. the examples of the failing int comparison in the feature file).