Change BrowserTestBase to flag PHPUnit failures instead of throwing Mink exceptions

Changing WebAssert::assert() is not enough: it will only fit for the assertions called within Drupal's override of the class. When we use methods defined on the parent Mink's WebAssert, the assert() method used is Mink's one, which throws Mink exceptions. So we would need to wrap the calls to such methods in try...catch blocks in the overrides of the parent methods.

on this

SAme as #4,

There's more to do, but since it's green now changing to NR to get feedback... does this make sense?

Thanks. I'm not aware of a mechanism to 'register' exceptions in PHPUnit so that they are interpreted as AssertionFailureErrors. Maybe that could be done with listeners in PHPUnit 9, but listeners are a kinda blind alley right now, they're deprecated and will no longer exist in PHPUnit 10, where they're replaced with the event system. So we need to wait and see I guess.

Nice try! The risk I see here is that overriding PHPUnit's methods we get exposed to future changes in PHPUnit itself.

For instance, in PHPUnit 10, PHPUnit\Framework\TestCase::addWarning() was made final so we can not override it any longer in Drupal\Tests\Traits\ PhpUnitWarnings. See #3217904-47: [meta] Support PHPUnit 10 in Drupal 11.

Actually this looks very very nice to me, kudos @longwave!

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -387,6 +389,24 @@ protected function setUp(): void {
+      $reflection->setAccessible(TRUE);

I think we do not need this with PHP 8.1.

Still a voice in the back of my head tells me to be aware this may change with PHPUnit 10, but let's see that when time comes.

IS needs an update for the new direction.

... and then I think it would be worth un-doing here the try-catch blocks in Drupal's WebAssert extension that are therefore no longer needed.

Thanks a lot for opening the upstream issue @longwave.

On this.

Fixed #16 and #18. Moved the onNotSuccessfulTest() implementation to a trait, see #3285270-3: Allow core test to only skip tests when a fragile part failed why.

Thinking again, the possible issue with this approach is that we will not increase the PHPunit assertions count - maybe we could, but only on failure, if we use Assert::fail() instead of just throwing the exception, but again we would lose the stack trace IIIC. And we may end up with successful tests reported as 'risky' if only WebAssert assertions are part of a test.

Ah, cool then I think #2805849: WebAssert does not track assertions, Test marked as risky no longer stands. addToAssertionCount has been marked @internal and will be final in PHPUnit 10, but still as long as we do not want to override it should be ok. Thanks and sorry for the noise.

Those names would be Mink specific though, and we will have another headache with #3285270: Allow core test to only skip tests when a fragile part failed which is up for something different still out of that same callback.

More general name? or a trait that somehow calls other traits?

How about something like this?

It's there as a structural element, to avoid we do not proliferate alternative implementations of the callback.

In #3285270: Allow core test to only skip tests when a fragile part failed we can make it 'do something' as the skipping would be applicable to all test suites. Then non-BTB test suites can implement an empty unsuccessfulTestHandler() in the base class.

Please see #39. If rationale there is not enough, happy to revisit.

Re #41,

I also think we should push harder upstream.

Upstream is a no-no. https://github.com/sebastianbergmann/phpunit/issues/4983#issuecomment-12...

I was also thinking - instead of banging our heads against PHPUnit, shall we try Mink instead? I.e. make it possible for WebAssert to specify a callback to manage the assertion, and implement it with PHPUnit's assertTrue(). And only if no callback specified, fall back to the private behaviour

    /**
     * Asserts a condition.
     *
     * @param bool   $condition
     * @param string $message   Failure message
     *
     * @throws ExpectationException when the condition is not fulfilled
     */
    private function assert($condition, $message)
    {
        if ($condition) {
            return;
        }

        throw new ExpectationException($message, $this->session->getDriver());
    }

?

Filed https://github.com/minkphp/Mink/issues/835 upstream, when I find time I will try a PR.

So, also Mink is pushing back hinting at a solution that is what the MR actually implements here, more or less, https://github.com/minkphp/Mink/issues/835#issuecomment-1260726225

Back to square one...

I think in the end the solution here is the correct one, but the fact that uses reflection to push the Mink exception trace into PHPUnit internals which is likely not sustainable.

The problem is a PHPUnit's one though, not Drupal's.

So I looked into trying to fix the exception chaining trace issue in PHPUnit AssertionFailedError. With the patch attached here applied to PHPUnit, the MR here applied to Drupal, and NodeTitleTest broken like in #14, now the PHPUnit output will show

PHPUnit 9.5.24 #StandWithUkraine

Runtime:       PHP 8.1.10
Configuration: /home/runner/work/d8-unit/d8-unit/core/phpunit.xml.dist

Testing Drupal\Tests\node\Functional\NodeTitleTest
F                                                                   1 / 1 (100%)

Time: 00:06.500, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\node\Functional\NodeTitleTest::testNodeTitle
Title found

/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/BrowserTestBase.php:441
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/Traits/UnsuccessfulTestHandlerTrait.php:16
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Caused by Behat\Mink\Exception\ExpectationException: Title found
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/WebAssert.php:538
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/WebAssert.php:281
/home/runner/work/d8-unit/d8-unit/core/modules/node/tests/src/Functional/NodeTitleTest.php:94
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

FAILURES!
Tests: 1, Assertions: 17, Failures: 1.

Any feedback before I start the quest to get the change into PHPUnit?

Filed the https://github.com/sebastianbergmann/phpunit/issues/5068 issue upstream, will follow with a PR based on #51.

PR upstream https://github.com/sebastianbergmann/phpunit/pull/5069

PHPUnit upstream rejected to implement #5069 and will not work on #5068, so we're on our own again. I will try something else soon.

This is passing the parent stack trace as part of the AssertionFailedError message, and avoid exception chaining that is broken in PHPUnit for tests running in isolation.

For reference, with the patch here and a broken NoteTitleTest like in #14, we get

Run vendor/bin/phpunit -v -c core --color=always core/modules/node/tests/src/Functional/NodeTitleTest.php
  
PHPUnit 9.5.24 #StandWithUkraine

Runtime:       PHP 8.1.11
Configuration: /home/runner/work/d8-unit/d8-unit/core/phpunit.xml.dist

Testing Drupal\Tests\node\Functional\NodeTitleTest
F                                                                   1 / 1 (100%)

Time: 00:12.737, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\node\Functional\NodeTitleTest::testNodeTitle
Title found
Caused by Behat\Mink\Exception\ExpectationException
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/WebAssert.php:538
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/WebAssert.php:281
/home/runner/work/d8-unit/d8-unit/core/modules/node/tests/src/Functional/NodeTitleTest.php:93
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/BrowserTestBase.php:444
/home/runner/work/d8-unit/d8-unit/core/tests/Drupal/Tests/Traits/UnsuccessfulTestHandlerTrait.php:16
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

FAILURES!
Tests: 1, Assertions: 17, Failures: 1.

Working on @alexpott's feedback.

Maybe this is just simpler?

fixed

@alexpott we probably do not need a trait then, is that OK?

So, here changed to a trait that only manages the conversion of the Mink exception to a PHPUnit failure - that can be reused elsewhere, and left the onNotSuccessfulTest() implementation in BrowserTestBase.

I think @longwave mentioned trying that #13 and #14, we'd have to extend AssertionFailedError here which would throw an error via SymfonyDebugClassloader since that's @internal.

But yes, maybe we no longer need a trait at this point of the MR, we could just have an abstract class with a static method to convert the Mink exception to a failure.

Thoughts?

Yes, that's why this issue was in 'needs review'. Changing to 'needs work' does not help here since we do not know what work needs to be done.

Yes, I will be happy to do that when I get feedback on #65. I have concerns that bumping the branch for its own sake is not very productive.

Bumped to 10.1.x and refactored to remove the need of the trait. We can't do #63 unless we break a PHPUnit @internal exception which is not good for future compatibility.

rebased

FTR, in PHPUnit 10.1 this might be addressed via https://github.com/sebastianbergmann/phpunit/issues/5201

quite amusing

agree :)

waiting until we update to PHPUnit 10.1 and can use the new capabilities

agree... this is an improvement but not strictly necessary. Can wait.

reparenting to PHPUnit 10 meta

Closing as duplicate, will be part of #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency. See https://git.drupalcode.org/project/drupal/-/merge_requests/6326/diffs?co...