Add support for Phpunit 10

[Jean85]

This is a rework of #41, after #43.

As @stof noted there, now the CI is highlighting that, under PHPUnit 10, Prophecy is behaving badly, and rendering an error instead of a failure when a Spy detects an unmet expectation.

Apart from that, this PR is complete; I'll try to delve a bit, to understand if that has to be fixed here or on Prophecy.

PHPUnit 10.1 is now released, including sebastianbergmann/phpunit#5201, and this PR is ready!

[Jean85]

It seems that the only point where a failure is triggered in the whole PHPUnit 10 codebase is here: https://github.com/sebastianbergmann/phpunit/blob/250b8d3e9fc5c27a4947291a6bec720eba015f07/src/Framework/TestCase.php#L639-L646

It's a catch block that catches either AssertionError (which is triggered by an assert(...) failure) or or AssertionFailedError.

The first one depends on ini settings to work as expected, and the latter is declared @internal, so it should not be relied upon, but we already do in this package.

The exception that is triggering the error in this case has this stack trace:

prophecy-phpunit/vendor/phpspec/prophecy/src/Prophecy/Prediction/CallPrediction.php:62
prophecy-phpunit/vendor/phpspec/prophecy/src/Prophecy/Prophecy/MethodProphecy.php:436
prophecy-phpunit/vendor/phpspec/prophecy/src/Prophecy/Prophecy/MethodProphecy.php:458
prophecy-phpunit/fixtures/SpyFailure.php:18
prophecy-phpunit/tests/run_test.php:23

so, the only way that I can think of to fix this is:

• at every prophesize(...) call, return a decorated ObjectProphecy...
• ...which in turn, at every __call invocation, returns a decorated MethodProphecy...
• ... that wraps every should...BeenCalled in a try { ... } catch { throw new AssertionFailedError }
  It's a bit convoluted... @stof can you think of any other way? Should I proceed like that?

[Jean85]

Ok, that doesn't seem feesible: MethodProphecy is hardly decorable due to its numerous methods and no interface, and MethodProphecy::__call has some internal logic that is hard do extract.

Any other pointer?

[icanhazstring]

AFAICT Prophecy is using PHPUnit 9 for require-dev.
I think this is something to be fixed in prophecy itself to require PHPUnit 10 as well to handle this properly?

[stof]

@icanhazstring I don't understand your comment. I don't see how the dev requirement of prophecy is related to the discussion happening here.

@Jean85 As discussed in the previous PR on that topic, I think we are missing a PHPUnit extension point here to be able to implement that feature.

[Jean85]

@Jean85 As discussed in the previous PR on that topic, I think we are missing a PHPUnit extension point here to be able to implement that feature.

I don't fully agree: the extension point is probably already there, and it's either:

• the event system, by dispatching an AssertionFailed event
• the old approach, throwing (or re-throwing) an AssertionError

The impediment here is in Prophecy, that is not easily extendable/decorable at this point.

[stof]

@Jean85 To me, the issue is that PHPUnit does not allow to easily convert external assertion failures to a PHPUnit assertion. Forcing Prophecy to directly throw an AssertionFailedError exception would require to couple Prophecy itself to PHPUnit, which is a no-go (it is also a no-go for most external assertions libraries btw. Drupal had the same issue about Mink assertions marking tests as errored instead of failed).
And forcing to use AssertionError as the only alternative looks also bad:

• AssertionError is explicitly documented by PHP as AssertionError is thrown when an assertion made via assert() fails., which might not really be OK if we use it as a base class for assertion thrown elsewhere
• AssertionError forces changing the base class (it is an Error, not an Exception), which cannot be done outside a major version of those libraries.
  And triggering an AssertionFailed event ourselves won't work. This event allows listener to react to a failure. It is not a way for custom assertions to report a test as failed vs errored as it is not a way to set the test status.

Note that in the past, TestCase::onNotSuccessfulTest was a solution for a base testcase to do this rethrowing (with drawbacks as an AssertionFailedError would not preserve its previous exception when serialized by process-isolated tests, making it impossible to see the original stack trace in such tests). However, looking at the PHPUnit 10 codebase, it seems like this method is now called after determining the test status, so this method cannot be used for anything that should change the status anymore (which makes me dubious about what this method is usable for)

[icanhazstring]

@stof sorry, misunderstook how prophecy is integrated into phpunit.
Found it out now. Sorry for the weird message ;)

As part of the change needed.
I would vote to have a major release, changing to event-based implementation for prophecy-phpunit.

[stof]

@icanhazstring From what I saw until now (I might be wrong as I haven't spent a long time on that yet), I don't think the new event system would help us here to mark a test catching a PredictionException as failed instead of errored. AFAICT, listeners are not allowed to change the test status, they can only read it.

[icanhazstring]

Same for me. Haven't digged deeper into that. But would make sense. Maybe someone who wrote that could have more input on that.

Will ask around a little.

[Jean85]

I'm doing quite some work with PHPUnit events, and I can try to weigh in a little.

AFAICT, listeners are not allowed to change the test status, they can only read it.

You don't change test status with a listener, but you can try to dispatch an event (using PHPUnit\Event\Emitter) and let PHPUnit's subscribers change the test results. The whole system is wrote that way, and test status is handled through events. As an example, a test failure does this path:

• catch AssertionError|AssertionFailedError and call \PHPUnit\Event\Emitter::testFailed https://github.com/sebastianbergmann/phpunit/blob/e00f0fe9220172ca867b2a3ff0bc2b291d07dfc3/src/Framework/TestCase.php#L639-L646
• call \PHPUnit\Event\Dispatcher::dispatch with Test\Failed event https://github.com/sebastianbergmann/phpunit/blob/e00f0fe9220172ca867b2a3ff0bc2b291d07dfc3/src/Event/Emitter/DispatchingEmitter.php#L602-L612
• \PHPUnit\TestRunner\TestResult\TestFailedSubscriber intercepts the event https://github.com/sebastianbergmann/phpunit/blob/main/src/Runner/TestResult/Subscriber/TestFailedSubscriber.php
• \PHPUnit\TestRunner\TestResult\Collector::testFailed collects the outcome

Is this enough?

[stof]

@Jean85 but those catching are done in TestCase::runBare, and our trait cannot customize what is caught there when a PredictionException happens in Prophecy.
With the current non-extensible logic about determining failures, this exception would be caught by https://github.com/sebastianbergmann/phpunit/blob/e00f0fe9220172ca867b2a3ff0bc2b291d07dfc3/src/Framework/TestCase.php#L649-L657 instead that reports an error and not a failure. By that time, it is too late for our trait to do anything about the status, even if we were to take ownership of the onNotSuccessfulTest method (which has no equivalent hook that would allow the trait to register a hook without conflict with classes consuming the trait).

[Jean85]

Understood. Well, I fear that we'll need @sebastianbergmann to weigh in and suggest us the best course of action.

Sebastian, a TL;DR for you: in Prophecy, when a spy fails its prediction (->shouldHaveBeenCalled...()) an exception is thrown, which was catched by PHPUnit 9 and transformed into a test failure (and not error).

With PHPUnit 10 this is gone, and we can't think of a way to fix this.

[sebastianbergmann]

Are you asking for a marker interface that is provided by PHPUnit and when an exception that implements this marker interface is raised during a test then that exception is to be treated as a failure?

[icanhazstring]

Another exception that can be used by libraries to mark a test as failed could also work I suppose.

[sebastianbergmann]

I have created sebastianbergmann/phpunit#5201 to track this.

[Jean85]

I have created sebastianbergmann/phpunit#5201 to track this.

Thanks Sebastian!
@stof is this satisfactory for you? We could re-declare the marker interface in Prophecy if missing, so that we could avoid the dependency toward PHPUnit...

[stof]

@sebastianbergmann ideally, it would be great if it was not a marker interface coming from PHPUnit, as that would mean that Prophecy would have to depend on PHPUnit just to make PredictionException extend that marker interface (PredictionException is already our own marker interface in Prophecy). It would be great to have a way for a PHPUnit extension to register additional marker exceptions for that (this would also solve this for the case of Mink that I mentioned above btw).

An alternative way would be to implement that through an attribute instead of a marker exception (as such attribute does not introduce a hard coupling). But then, it would still be great to support adding the attribute on an interface or a parent class instead of having to put it directly on the class being triggered.

Another alternative is to make onNotSuccessfulTest work again to change the status of tests (see sebastianbergmann/phpunit#5202), potentially allowing hook methods for that extension point (so that the trait can register a hook without needing to take ownership of the onNotSuccessfulTest method name).

[stof]

We could re-declare the marker interface in Prophecy if missing, so that we could avoid the dependency toward PHPUnit...

This would encourage bad practices if the recommended way is that each library doing such assertions (Prophecy, but also Mockery, Mink, etc...) would re-declare the marker interface, because it means that PHPUnit would never be able to change that marker interface anymore without breaking the ecosystem badly (as those re-declarations would not automatically be in sync with PHPUnit).
So I suggest finding an extension point that does not require such hack instead.

[sebastianbergmann]

Ah, now I understand. Thanks!

[Jean85]

This would encourage bad practices if the recommended way is that each library doing such assertions (Prophecy, but also Mockery, Mink, etc...) would re-declare the marker interface, because it means that PHPUnit would never be able to change that marker interface anymore without breaking the ecosystem badly (as those re-declarations would not automatically be in sync with PHPUnit).

To be fair, I was thinking about a conditional redeclaration. Being a marker interface, it would be needed to be empty, any change would be a BC...

Still, registering marker interfaces or classes is a better approach 👍

[localheinz]

@Jean85

The build currently fails because of an authentication failure. Can you retry?

[Jean85]

I cannot.

Also, the failure comes from the fact that we need to install phpspec/prophecy#585, and stems from ramsey/composer-install#241.

The short term solution is to have a GitHub token added to the CI to pull the Prophecy fork in. Or we need to wait for that to be merged.

[vladdnepr]

Hi. Do you continue this feature?

[Jean85]

We're in a deadlock of PR to be merged... this one, phpspec/prophecy#585 and phpspec/phpspec#1444 :(

[Wirone]

FYI: I contacted @ciaranmcnulty and some of blocking PRs got merged, so there are only two more steps to provide PHPUnit 10 support 🙂 (this and phpspec/phpspec#1444). It needs to be verified, though.

[Jean85]

FINALLY!!! It's green and it's working!

We may have to wait for phpspec/prophecy#585 to be tagged, but it's ready!

[Jean85]

I've added a specific CI job to test against PHPUnit 10

[Jean85]

PHPUnit 10.1 is released, this PR is ready to go! 🎉

[stof]

Please also fix the conflicts

[darthf1]

Was this the last blocker for PHPUnit 10 compatibility? If so, is it possible to create a release including this PR?