Allow custom processing of WebAssert::assert through callback

[mondrake]

Right now, the WebAssert::assert() throws a ExpectationException when the condition is not fulfilled.

When running tests with PHPUnit + Mink, this results in PHPUnit 'errors' instead of 'fails', which is not correct, because

A failure is a violated PHPUnit assertion such as a failing assertSame() call. An error is an unexpected exception or a PHP error.

And when we assert with Mink, we check for a specific condition.

This came up in Drupal, see more details in Change BrowserTestBase to flag PHPUnit failures instead of throwing Mink exceptions.

It would be good to have the possibility to specify a callback that WebAssert::assert() could use, falling back to current behaviour if no callback is specified.

[stof]

instead of forcing to customize WebAssert (which also means we will never be able to add new kinds of failures in the proposed API as that would break BC for your callback), a cleaner solution would be to use the dedicated PHPUnit feature to handle some exceptions as failures.

See https://github.com/minkphp/driver-testsuite/blob/af6107c1f16f4c43c19ea162e0da88d55c6cf21a/tests/TestCase.php#L74-L81 where our driver testsuite uses that hook to mark tests as skipped instead of errored when a Behat\Mink\Exception\UnsupportedDriverActionException exception is thrown. You could do the same to turn Behat\Mink\Exception\ExpectationException into a failure instead

[alexpott]

Yes but doing that means re-throwing the exception from the onNotSuccessful and therefore changing the exception type. We have tried to propose changes to PHPUnit to facilitate this see sebastianbergmann/phpunit#4983 (comment)

Atm if we want to mark Mink exceptions as PHPUnit failures we have do something like:

    if ($t instanceof MinkException) {
      $e = new AssertionFailedError($t->getMessage(), $t->getCode(), $t);
      $trace = $t->getTrace();
      foreach ($trace as $i => $call) {
        unset($trace[$i]['args']);
      }
      $reflection = new \ReflectionProperty($e, 'serializableTrace');
      $reflection->setValue($e, $trace);
      throw $e;
    }

in order to preserve the trace. Which in my mind not very sensible.

[stof]

AFAIU, the need for this complex code is caused by AssertionFailedError loosing the chained exception when serializing it for process isolation. I would rather work on fixing that in PHPUnit (as chaining exceptions was the suggested solution in sebastianbergmann/phpunit#4983 (comment))

[mondrake]

I have filed sebastianbergmann/phpunit#5068 which, I think, would solve the trace issue while using the solution proposed here in #835 (comment) which seems to me the cleanest one.

[mondrake]

PHPUnit rejected sebastianbergmann/phpunit#5069 and will not pursue sebastianbergmann/phpunit#5068.

[stof]

Well, if he decided that mapping the details is the way to go, he should probably bundle an utility in PHPUnit to make it easy to copy the trace, given the hacks it requires. but that should be discussed with PHPUnit, as that's clearly a PHPUnit limitation here.

The proposal done here adds lots of complexity in Mink and means that we cannot add new assertions outside of major versions as it would change the requirements for that callback.

[mondrake]

Alright, I understand the position here. I will try something else on Drupal side, before calling time of death. Thanks

[stof]

sebastianbergmann/phpunit#5201 might allow to solve the root cause without the need for this weird feature request. So you can subscribe to that issue.

[mondrake]

Thanks for the pointer, subscribed there.

I’d say what’s weird here could be the solution - the feature request per se is quite sensible.