Problem/Motivation

PHPUnit 12 introduces additional checks on metadata attributes consistency.

Some data providers are generating more arguments than test methods expect. Looking in detail, some of them are bugs that should be fixed anyway.

Proposed resolution

Fix in advance.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3550292-nr-bot_suza0p9y.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3550292

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !13409

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake
Status: Needs review » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented

Looks like this was rebased at just the wrong moment today. #2353881: Add test coverage for file entity reference selection plugin broke head and that change was merged in to this issue's fork. It was fixed right away and I've started a rebase with that fix.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I'm guessing that a complete review of this would involve running all the tests with PHPUnit 12. I didn't do that. That would take all night on my laptop.

I did manual checks of the providers and the changes made to the test classes. Everything looks ok to me. The only bit of feedback that I had was for UnroutedUrlTest::testFromUri(). Its parameters didn't have the type hints added like all of the other test functions that are dependent on ::providerFromUri(), including ::testIsExternal() which had both of the parameters and required no modification. This is merely a lack of consistency and not something that I feel should hold up the issue. So I'm going to go ahead and mark this as RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

Thanks!

Since we are at it, lets’s do #8.

If you are intersted, the MR in the parent is running tests with PHPUnit 12. It’s my reference to see reported results that lead to issues like this one.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

If you are intersted, the MR in the parent is running tests with PHPUnit 12. It’s my reference to see reported results that lead to issues like this one.

Thank you for the tip!

My feedback was addressed. This is RTBC to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Test cleanup for PHPUnit 12 - unit tests data providers » Test cleanup for PHPUnit 12 - tests data providers
Assigned: Unassigned » mondrake
Status: Reviewed & tested by the community » Needs work

Let's extend to Kernel tests data providers, too - now we have a clean report of the failures in #3527936: Introduce support for PHPUnit 12.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Test cleanup for PHPUnit 12 - tests data providers » Test cleanup for PHPUnit 12 - round 2
Issue summary: View changes
Status: Needs work » Postponed
Related issues: +#3551681: Enforce removal of PHPUnit annotations from test classes' methods metadata
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Status: Postponed » Needs review

Blocker is in.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I manually verified all recent changes since my last review, comparing provider return values to the new test definitions. They all look good to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +beta target
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3550292-nr-bot_suza0p9y.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -beta target

rebased

  • longwave committed 1db7f532 on 11.x 
    test: #3550292 Test cleanup for PHPUnit 12 - round 2

By: mondrake
By:...

  • longwave committed 33a27cd6 on main 
    test: #3550292 Test cleanup for PHPUnit 12 - round 2

By: mondrake
By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Nice cleanup, I like the use of @final in UnitTestCaseTest to stop us extending it by mistake again!

Backported to 11.x to keep things in sync.

Committed and pushed 33a27cd673d to main and 1db7f532c7b to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13409

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.