Phpunit 10 support #41

Jean85 · 2023-01-11T23:07:29Z

This is a fast upgrade to make it work with PHPUnit 10; it also requires phpspec/prophecy#585

I had to migrate the testsuite to an end to end approach using PHPT tests, since PHPUnit 10 is completely overhauled and uses a new event dispatching system, that makes running tests inside tests a lot harder.

Also, I didn't wire Prophecy to the events that keep track of generated mocks & stubs, since those are designed of the PHPUnit mocks, which work differently (i.e. it allows partial mocks: https://github.com/sebastianbergmann/phpunit/blob/01b41781ada83e0719bbf350b43295ed90bd3ad9/src/Event/Events/Test/TestDouble/PartialMockObjectCreated.php#L21)

Jean85 · 2023-01-11T23:09:49Z

This should probably require a major bump, and be released (after PHPUnit 10, slated for February) as 2.0.

stof · 2023-01-12T10:15:46Z

xdebug_filter.php

+        XDEBUG_PATH_INCLUDE,
+        [ __DIR__ . DIRECTORY_SEPARATOR . 'src' . DIRECTORY_SEPARATOR ]
+    );
+    xdebug_set_filter(


Isn't this managed by PHPUnit already based on the configuration in phpunit.xml ? Or is it not supported for the phpt runner ?

I don't know exactly, maybe is due to PHPT; locally, as said in the commit message, with this I observed a reduction of execution time from 26 seconds to only 3.

stof · 2023-01-12T10:16:57Z

fixtures/WrongCall.php

@@ -5,7 +5,7 @@
 use PHPUnit\Framework\TestCase;
 use Prophecy\PhpUnit\ProphecyTrait;

-class Error extends TestCase
+class WrongCall extends TestCase


which possible naming collisions ? The namespace Prophecy\PhpUnit\Tests\Fixtures is fully ours.

I was getting a "Cannot find class Error" with that, and the simple renaming made it go away.

stof · 2023-01-12T10:19:02Z

src/ProphecyTrait.php

@@ -38,11 +38,6 @@ trait ProphecyTrait
     */
    protected function prophesize(?string $classOrInterface = null): ObjectProphecy
    {
-        if (\is_string($classOrInterface)) {
-            \assert($this instanceof TestCase);
-            $this->recordDoubledType($classOrInterface);


this seems unrelated to the commit message

It's not. recordDoubledType does not exist anymore on TestCase, this was the only blocker that was making it fatal during execution. It's all handled with events on PHPUnit now, but the events are strictly related to PHPUnit mocks, so I don't know if its sensible to use them; maybe we could tackle that in a separate PR?

The commit adding it says Add test to reach 100% coverage.

anyway, I suggest that increasing test coverage to 100% should be done in a separate PR so that we already get 100% test coverage with the code using PHPUnit 9.x

The commit adding it says Add test to reach 100% coverage.

Probably my mistake while committing

anyway, I suggest that increasing test coverage to 100% should be done in a separate PR so that we already get 100% test coverage with the code using PHPUnit 9.x

Done in #42

stof · 2023-01-12T10:31:59Z

Would it be possible to keep support for phpunit 9 and 10 in the same codebase instead of removing support for PHPUnit 9 ? AFAICT, the trait is the same in both cases so I don't see a need to double the maintenance work by maintaining 2 versions.

And I suggest extracting the testsuite conversion to phpt into a dedicated PR so that we don't have the testsuite rewrite at the same time than the support for PHPUnit 10

Jean85 · 2023-01-12T11:10:35Z

As said in another comment, cross compat is not feasible due to TestCase::recordDoubledType being dropped on PHPUnit 10. That was useful only for reporting, since it was recording how many mocks were being created, and logging it.

Since PHPUnit 10, that took a drastic turn and it's done with events; it's also shaped a lot more around PHPUnit's own mocking engine, due probably to Prophecy support being dropped.

At the end, a decision on how much integration we want to support is to be done, and that might shape the possibility of cross-support:

if we make the recordDoubledType conditional, it may still work cross-version, but it will provide stats only on 9.x
if we want to support stats on PHPUnit 10, we definitely need a new major

stof · 2023-01-12T12:01:40Z

if we want to support stats on PHPUnit 10, we definitely need a new major

I don't think we need. We would have a condition to decide between the 9.x and the 10.x way of reporting stats. That's all hidden inside the internals of the trait

stof · 2023-01-12T13:25:38Z

Thus, I'm not sure the stats about the doubled types are useful to keep. @sebastianbergmann do you have any insight about the cleaner solution for that ?

Jean85 · 2023-01-12T14:42:41Z

Thanks @stof for the thorough review. I've split the test coverage increase; I'll leave this PR as-is because it's useful for me, but I'm available for reworking it as needed.

And I suggest extracting the testsuite conversion to phpt into a dedicated PR so that we don't have the testsuite rewrite at the same time than the support for PHPUnit 10

I've tried doing it, it doesn't seems totally feasible: the entry point is profoundly different (different class), and even the end result of some cases is different (i.e. the spy failure case is handled as a failure in 9, error in 10).

stof · 2023-01-12T14:45:34Z

i.e. the spy failure case is handled as a failure in 9, error in 10

This looks bad to me. It might indicate a missing hook in PHPUnit or a missing usage of the hook.

stof · 2023-01-12T14:47:15Z

the entry point is profoundly different (different class)

you mean the entrypoint for running the test in the phpt ? This could be abstracted into an helper taking the testcase to run as argument and then doing the right execution depending on the PHPUnit version. That would still be less maintenance work than maintaining 2 versions of the library in parallel.

sebastianbergmann · 2023-01-12T15:08:42Z

The only sensible thing I can suggest is to not support PHPUnit anymore, or at least not as deeply (counting expectations) as before. I am sorry, but I lack the time to look into Prophecy and provide more useful feedback/suggestions.

stof · 2023-01-12T15:42:48Z

@sebastianbergmann exposing extension point to easily mark some exceptions as being failures rather than errors would be enough for us to integrate cleanly for that case of spy failures being reported differently when using 9.x or 10.x.

When building this extension for PHPUnit 9, several extension points (like the @postCondition annotation) were added in PHPUnit to unlock such kind of traits. The case of spy failures was probably missed because PHPUnit 9.x defines the Prophecy PredictionException as a failure in core.

I can imagine 2 different ways this could look like (maybe there are more):

a way to register some base type that would report those exceptions as failure in PHPUnit (similar to how the core AssertionFailedError is treated but allowing other types to trigger that code path). However, this seems to have been rejected in Allow specific exceptions to be marked as failures instead of errors sebastianbergmann/phpunit#4983
a way to convert exceptions into AssertionFailedError when they should be failures. This would be similar to what onNotSuccessfulTest allows to do right now, but allowing a metadata-based registration of a method would make the DX better in such a trait than supporting only that specific name. Maybe we could have a @onNotSuccessful annotation (or probably an attribute instead) supporting that case. And for a good experience when using process isolation, it might require fixing the case described in Allow specific exceptions to be marked as failures instead of errors sebastianbergmann/phpunit#4983 (comment): AssertionFailedError looses its chained exception (and so the useful part of the stack trace) when serializing it to send it to the main process

This would allow to keep the proper distinction between a failure and an error (which makes sense, otherwise PHPUnit would have dropped that distinction for its own code to simplify it) when assertions are not done directly with the PHPUnit assertion system.

For the case of stats about double types, I'm totally fine about dropping them (I don't even know how those stats are reported as they don't seem to impact the default output of PHPUnit)

I've tried doing it, it doesn't seems totally feasible: the entry point is profoundly different (different class), and even the end result of some cases is different

@Jean85 that's actually one more argument to perform the PHPT conversion separately for the existing codebase first. This way, changes to the tests would appear in the diff of the PR. In the current state of the PR, I did not notice that the converted tests were not performing the same assertions.

aik099 · 2023-01-12T15:48:01Z

@stof , what about #38 ? I'd rather see it merged and released and based on its code make changes for PHPUnit 10 support.

stof · 2023-01-12T16:01:53Z

With PHPUnit 8 reaching EOL in 3 weeks, I'm not sure it is worth to have #38 which makes the code much more complex (at least to understand)

Jean85 · 2023-01-12T21:08:40Z

@Jean85 that's actually one more argument to perform the PHPT conversion separately for the existing codebase first. This way, changes to the tests would appear in the diff of the PR. In the current state of the PR, I did not notice that the converted tests were not performing the same assertions.

Done in #43 👍

phil-davis · 2023-02-03T10:20:15Z

Note: phpunit 10 was just officially released.
I was just having a look at making PHP-CS-fixer use phpunit 10 (so that I can then use that in other things...). It depends on prophecy-phpunit, so needs this PR to happen first. And might want phpspec/prophecy#585 also (not sure).

There is going to be a big cascade of stuff that needs to happen to get phpunit 10 enabled in the PHP world!

Jean85 · 2023-02-03T12:05:47Z

Right.

This PR has to be totally revisited since #43 got merged.

Jean85 · 2023-02-03T21:40:40Z

Closing in favor of #45.

Jean85 added 8 commits January 11, 2023 21:56

Require PHPUnit 10 (dev)

68a9cea

Migrate PHPUnit config

58759fc

Allow Prophecy fork to work with PHPUnit 10

4b84620

Rename fixture (due to possible naming collision)

ae5783a

Migrate test suite to PHPT/E2E approach

04ef895

Add test to reach 100% coverage

03f5fa5

Implement Xdebug filtering for faster execution (26s to 3s locally)

bbdeb45

Bump PHP requirement to 8.1 (same as PHPUnit)

f76d29e

Jean85 mentioned this pull request Jan 11, 2023

Refactor log parser, adapt to new events facile-it/paraunit#172

Merged

stof requested changes Jan 12, 2023

View reviewed changes

Jean85 mentioned this pull request Jan 12, 2023

Add test to reach 100% coverage #42

Merged

Jean85 mentioned this pull request Jan 12, 2023

Switch to E2E PHPT tests #43

Merged

Jean85 mentioned this pull request Feb 3, 2023

Add support for Phpunit 10 #45

Merged

Jean85 closed this Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phpunit 10 support #41

Phpunit 10 support #41

Jean85 commented Jan 11, 2023

Jean85 commented Jan 11, 2023

stof Jan 12, 2023

Jean85 Jan 12, 2023

stof Jan 12, 2023

Jean85 Jan 12, 2023

stof Jan 12, 2023

Jean85 Jan 12, 2023

stof Jan 12, 2023

Jean85 Jan 12, 2023 •

edited

Loading

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

stof commented Jan 12, 2023

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

stof commented Jan 12, 2023

stof commented Jan 12, 2023

sebastianbergmann commented Jan 12, 2023

stof commented Jan 12, 2023

aik099 commented Jan 12, 2023 •

edited

Loading

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

phil-davis commented Feb 3, 2023

Jean85 commented Feb 3, 2023

Jean85 commented Feb 3, 2023

Phpunit 10 support #41

Phpunit 10 support #41

Conversation

Jean85 commented Jan 11, 2023

Jean85 commented Jan 11, 2023

stof Jan 12, 2023

Choose a reason for hiding this comment

Jean85 Jan 12, 2023

Choose a reason for hiding this comment

stof Jan 12, 2023

Choose a reason for hiding this comment

Jean85 Jan 12, 2023

Choose a reason for hiding this comment

stof Jan 12, 2023

Choose a reason for hiding this comment

Jean85 Jan 12, 2023

Choose a reason for hiding this comment

stof Jan 12, 2023

Choose a reason for hiding this comment

Jean85 Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

stof commented Jan 12, 2023

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

stof commented Jan 12, 2023

stof commented Jan 12, 2023

sebastianbergmann commented Jan 12, 2023

stof commented Jan 12, 2023

aik099 commented Jan 12, 2023 • edited Loading

stof commented Jan 12, 2023

Jean85 commented Jan 12, 2023

phil-davis commented Feb 3, 2023

Jean85 commented Feb 3, 2023

Jean85 commented Feb 3, 2023

Jean85 Jan 12, 2023 •

edited

Loading

aik099 commented Jan 12, 2023 •

edited

Loading