Support PHPUnit 10 #70

spawnia · 2023-02-03T10:58:49Z

The changes in ArraySubsetAsserts were made due to the following test failures:

There were 2 failures:

1) DMS\PHPUnitExtensions\ArraySubset\Tests\Unit\AssertTest::testAssertArraySubsetThrowsExceptionForInvalidSubsetArgument
Failed asserting that exception of type "Error" matches expected exception "PHPUnit\Framework\Exception". Message was: "Call to undefined method PHPUnit\Framework\InvalidArgumentException::create()" at
/home/bfranke/projects/phpunit-arraysubset-asserts/src/ArraySubsetAsserts.php:35
/home/bfranke/projects/phpunit-arraysubset-asserts/tests/Unit/AssertTest.php:32
.

2) DMS\PHPUnitExtensions\ArraySubset\Tests\Unit\AssertTest::testAssertArraySubsetThrowsExceptionForInvalidArrayArgument
Failed asserting that exception of type "Error" matches expected exception "PHPUnit\Framework\Exception". Message was: "Call to undefined method PHPUnit\Framework\InvalidArgumentException::create()" at
/home/bfranke/projects/phpunit-arraysubset-asserts/src/ArraySubsetAsserts.php:51
/home/bfranke/projects/phpunit-arraysubset-asserts/tests/Unit/AssertTest.php:38

Awaiting rdohms/phpunit-arraysubset-asserts#70

jrfnl · 2023-02-04T00:31:02Z

Looks like you are removing bits and pieces which ensured compatibility with PHPUnit 8.x. I very much doubt CI will pass on this PR.

spawnia · 2023-02-04T15:08:04Z

Looks like you are removing bits and pieces which ensured compatibility with PHPUnit 8.x. I very much doubt CI will pass on this PR.

Indeed, there are behavioural changes. I believe there is little value in maintaining the different "original" error messages or exception types (if there even is such a thing, given this feature has been missing from PHPUnit for quite some time). It is not like someone is going to build functionality around what specifically happens when assertArraySubset() is misused in a certain way.

I don't see any incompatibility being introduced. The functionality can be used just like before, and the new error message should be just as clear as the one that was previously given.

Tests passed locally.

jrfnl · 2023-02-04T18:15:47Z

Tests passed locally.

@spawnia Against which PHP/PHPUnit combinations ?

mattvb91 · 2023-02-07T08:29:21Z

@spawnia can you trigger the tests in your repo?

spawnia · 2023-02-07T10:17:07Z

I was able to run the tests in my fork and fixed a few minor issues that prevented CI from working, see https://github.com/spawnia/phpunit-arraysubset-asserts/pull/1.

However, there is still a CI failure due to usage of a deprecated configuration schema, see https://github.com/spawnia/phpunit-arraysubset-asserts/actions/runs/4112652928/jobs/7097833112. I tried to fix this with b5299be (#70), but apparently this did not help. I was able to fix the issue by basically miniziming the PHPUnit configuration to avoid usage of incompatible elements (e.g. whitelist was changed to include): 97c61a5 (#70).

mattvb91 · 2023-02-20T11:05:19Z

@jrfnl @rdohms any idea what is needed to get this in? Thanks

jrfnl · 2023-02-20T19:12:54Z

@jrfnl @rdohms any idea what is needed to get this in? Thanks

Time for the maintainers to do a proper review. I'll do one once I'm back from my break, but that will still be a couple more weeks.

jrfnl

FWIW: I've done a detailed review of this PR.

In my personal opinion, this PR should not be merged as-is. Please see the inline comments for more detailed feedback.

.github/workflows/run-tests.yml

composer.json

composer.lock

jrfnl · 2023-03-20T05:42:26Z

phpunit.xml.dist

-<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-         xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
-         colors="true"
-         verbose="true"
+<phpunit colors="true"
+         failOnWarning="false"
         beStrictAboutOutputDuringTests="true"
-         beStrictAboutTodoAnnotatedTests="true"
         beStrictAboutChangesToGlobalState="true"
-         convertErrorsToExceptions="true"
-         convertWarningsToExceptions="true"
-         convertNoticesToExceptions="true"
-         convertDeprecationsToExceptions="true"
-         bootstrap="tests/bootstrap.php"
->


It's unclear to me why the majority of these settings are being removed.

AFAICS, the only change needed is: xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.2/phpunit.xsd" to indicate to PHPUnit the schema used (with 9.2 deliberately chosen as PHPUnit changes some thing in PHPUnit 9.3).

The original schema setting made it dependent on the Composer installed PHPUnit version, which makes the schema setting unstable if PHPUnit 10.0 would also be supported (as PHPUnit 10 made significant changes to the schema).

As for the other values - in part - being set to the default value used by PHPUnit: those defaults have changed for a lot of these across PHPUnit versions, which is why it is important to have these settings explicitly set to make sure tests are being run with the same configuration across PHPUnit versions.

I reduced the PHPUnit configuration to be as minimal as possible in order to maximize compatbility between the different supported versions. Fortunately, the schema is not required and can simply be omitted.

I reduced the PHPUnit configuration to be as minimal as possible in order to maximize compatbility between the different supported versions. Fortunately, the schema is not required and can simply be omitted.

Unfortunately, minimizing the config does the opposite as it does not allow for the tests to run with the same config across various PHPUnit versions.

As for the other values - in part - being set to the default value used by PHPUnit: those defaults have changed for a lot of these across PHPUnit versions, which is why it is important to have these settings explicitly set to make sure tests are being run with the same configuration across PHPUnit versions.

Do you have a suggestion on how to fix https://github.com/spawnia/phpunit-arraysubset-asserts/actions/runs/4466219397/jobs/7844126983 then?

Of course 😉

I believe this can be fixed by running phpunit --migrate-configuration in a separate CI step prior to running the actual tests.
The step should be run conditionally for PHPUnit 10 only.

You can grab the PHPUnit version with something like this:

- name: Grab PHPUnit version id: phpunit_version run: echo "VERSION=$(vendor/bin/phpunit --version | grep --only-matching --max-count=1 --extended-regexp '\b[0-9]+\.[0-9]+')" >> $GITHUB_OUTPUT

... which allows you to check the version in a conditional step after that like so:

if: ${{ steps.phpunit_version.outputs.VERSION >= '10.0' }}

As a side-note: I find it weird that PHPUnit would exit with a non-zero exit code when it is only a warning from PHPUnit 10 itself and there are no test failures.
Might be worth opening an issue in the PHPUnit repo about this if there isn't one open already) as this is undesired and unexpected behaviour.

I could not get your suggestion to work, but found an alternative.

I've been doing some testing with the --migrate-configuration option and have found that unfortunately it does not sufficiently convert the configuration.

Most notably, the PHPUnit 10.x displayDetailsOn* settings do not get set, while for a tool like this, which is used in the test pipeline by other projects, it is important to have access to information about deprecations in new PHP versions early.

For the PHPUnit Polyfills, I've now solved this by having a separate config file for PHPUnit 10.
If you'd like to see the implementation as an example, please have a look at: Yoast/PHPUnit-Polyfills#111

Just noticed that in the upcoming PHPUnit 10.1.0 release there will be new failOnWarning, failOnNotice and failOnDeprecation attributes. Those seem even better/more appropriate.

See:

Optionally (fail|stop) on (notice|deprecation) events sebastianbergmann/phpunit#5196

sebastianbergmann/phpunit@fb6673f

We should tackle 10.1 in another PR and continue this discussion there.
WDYT?

src/ArraySubsetAsserts.php

tests/Unit/AssertTest.php

hilioski · 2023-04-04T13:58:41Z

Any updates on this PR?

rdohms

Let's cater to @jrfnl comments and then clean up this branch. It has a lot of back-and-forth commits.
I'm happy to do that since I took so long to review it. I need some help understanding the Exception issue.

src/ArraySubsetAsserts.php

composer.json

We no longer rely on the internal InvalidArgumentException from PHPUnit, but we still keep previous behavior for polyfill reasons. Co-authored-by: Benedikt Franke <benedikt.franke@mll.com>

rdohms · 2023-04-22T22:23:19Z

Alright, I did some cleanup and adjusted the Exception checks.
It should be all good now; @jrfnl can you re-review to see if I missed anything?

rdohms · 2023-04-22T22:34:11Z

I also dropped the composer.lock once and for all.

spawnia

Thanks @rdohms for getting into this, I am fine with the changes you made and hope it can get merged soon.

dramamask · 2023-05-09T20:50:52Z

I hope @jrfnl can review this soon. I'd love to keep using this package.

slaff · 2023-05-15T08:01:10Z

@jrfnl any chance of reviewing and merging this PR?

GityaMan · 2023-05-18T02:23:34Z

@jrfnl Is this package supported?

alexw23 · 2023-05-25T04:04:31Z

Didn't see this getting merged soon - using the help of Github Copilot was able to rewrite our 4 lines using this and remove an additional package.

spawnia · 2023-06-05T07:29:27Z

Thanks for the merge, waiting for the release now.

spawnia added a commit to webonyx/graphql-php that referenced this pull request Feb 3, 2023

Use PHPUnit 10

d7e04a9

Awaiting rdohms/phpunit-arraysubset-asserts#70

spawnia mentioned this pull request Feb 3, 2023

Use PHPUnit 10 webonyx/graphql-php#1300

Merged

rdohms requested a review from jrfnl February 14, 2023 12:56

jrfnl reviewed Mar 20, 2023

View reviewed changes

jrfnl mentioned this pull request Mar 31, 2023

PHPUnit 10 Compatibility #74

Closed

rdohms reviewed Apr 22, 2023

View reviewed changes

src/ArraySubsetAsserts.php Show resolved Hide resolved

composer.json Outdated Show resolved Hide resolved

Add Support for PHPUnit 10

815b4a6

We no longer rely on the internal InvalidArgumentException from PHPUnit, but we still keep previous behavior for polyfill reasons. Co-authored-by: Benedikt Franke <benedikt.franke@mll.com>

rdohms requested a review from jrfnl April 22, 2023 22:23

rdohms approved these changes Apr 22, 2023

View reviewed changes

spawnia commented May 3, 2023

View reviewed changes

rdohms merged commit aa6b9e8 into rdohms:master Jun 2, 2023

spawnia deleted the support-phpunit-10 branch June 5, 2023 07:27

spawnia mentioned this pull request Jun 5, 2023

Use PHPUnit 10 for testing nuwave/lighthouse#2409

Merged

rdohms added this to the v0.5.0 milestone Jun 7, 2023

rdohms added the Feature New feature or request label Jun 7, 2023

rdohms added the dependencies label Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PHPUnit 10 #70

Support PHPUnit 10 #70

spawnia commented Feb 3, 2023

jrfnl commented Feb 4, 2023

spawnia commented Feb 4, 2023

jrfnl commented Feb 4, 2023

mattvb91 commented Feb 7, 2023

spawnia commented Feb 7, 2023

mattvb91 commented Feb 20, 2023

jrfnl commented Feb 20, 2023

jrfnl left a comment

jrfnl Mar 20, 2023

spawnia Mar 20, 2023

jrfnl Mar 20, 2023

spawnia Mar 20, 2023

jrfnl Mar 20, 2023

spawnia Mar 20, 2023

jrfnl Apr 1, 2023

jrfnl Apr 2, 2023

rdohms Apr 22, 2023

hilioski commented Apr 4, 2023

rdohms left a comment

rdohms commented Apr 22, 2023

rdohms commented Apr 22, 2023

spawnia left a comment

dramamask commented May 9, 2023

slaff commented May 15, 2023

GityaMan commented May 18, 2023

alexw23 commented May 25, 2023 •

edited

Loading

spawnia commented Jun 5, 2023

Support PHPUnit 10 #70

Support PHPUnit 10 #70

Conversation

spawnia commented Feb 3, 2023

jrfnl commented Feb 4, 2023

spawnia commented Feb 4, 2023

jrfnl commented Feb 4, 2023

mattvb91 commented Feb 7, 2023

spawnia commented Feb 7, 2023

mattvb91 commented Feb 20, 2023

jrfnl commented Feb 20, 2023

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hilioski commented Apr 4, 2023

rdohms left a comment

Choose a reason for hiding this comment

rdohms commented Apr 22, 2023

rdohms commented Apr 22, 2023

spawnia left a comment

Choose a reason for hiding this comment

dramamask commented May 9, 2023

slaff commented May 15, 2023

GityaMan commented May 18, 2023

alexw23 commented May 25, 2023 • edited Loading

spawnia commented Jun 5, 2023

alexw23 commented May 25, 2023 •

edited

Loading