Problem/Motivation

phpunit/phpunit released 10.5.30. (https://github.com/sebastianbergmann/phpunit/releases/tag/10.5.30)

In this release, there's a new check if willReturnOnConsecutiveCalls() has the minimum required amount of return values defined, if a not the new \PHPUnit\Framework\MockObject\NoMoreReturnValuesConfiguredException is thrown. (https://github.com/sebastianbergmann/phpunit/commit/490879817a1417fd5fa1...)

In core this happens on one on occasion:

Drupal\Tests\Core\Asset\AssetResolverTest::testGetJsAssets with data set "different libraries, same timestamps"
PHPUnit\Framework\MockObject\NoMoreReturnValuesConfiguredException: Only 6 return values have been configured for Drupal\Core\Asset\LibraryDiscovery::getLibraryByName()

core/lib/Drupal/Core/Asset/AssetResolver.php:264
core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php:230

Steps to reproduce

See first test failure on https://git.drupalcode.org/issue/drupal-3468781/-/pipelines/257385/test_...
(Other failures are being handled in #3468502: sebastianbergmann/comparator:5.0.2 Introduces (for Drupal) breaking changes)

Proposed resolution

- Fix the sole occurrence of NoMoreReturnValuesConfiguredException in AssetResolverTest
- Bump version of phpunit/phpunit to latest in composer.* in a new issue.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3468781

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:

Comments

Spokje created an issue. See original summary.

spokje’s picture

Unsure if this is a bug or a task. Bug because now

AssetResolverTest::testGetJsAssets

is passing without enough return-values in it's Mock, so the test is untrustworthy. Task because that's what we usually do for breaking changes in a dependency update.

Went with task for now.

spokje’s picture

Issue summary: View changes
spokje’s picture

Status: Active » Needs work

By the power of almost Sherlock Holmes-like deduction, blunt trail-and-error it seems that $this->libraryDiscovery->expects($this->any())->method('getLibraryByName')->willReturnOnConsecutiveCalls() in Drupal\Tests\Core\Asset\AssetResolverTest::testGetJsAssets needs to return twelve instead of the current six values.

I'll leave it to bigger brains what those values should be.

spokje’s picture

Issue summary: View changes

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

catch’s picture

Status: Needs work » Needs review
spokje’s picture

Title: phpunit/phpunit:10.5.30 Introduces (for Drupal) breaking changes » [PP-1] phpunit/phpunit:10.5.30 Introduces (for Drupal) breaking changes
Status: Needs review » Postponed

Thanks @catch.

The test-failure I've opened this issue for is now indeed gone.
However, the version bump in phpunit/phpunit also causes a version bump in sebastianbergmann/comparator, which causes test-failures.
Those are being handled in[#3468502].

So I think the right thing to do now is postponing this issue on that one landing.

mondrake’s picture

Status: Postponed » Needs work

Can’t the AssetResolverTest fix be done now, independently from the dependency bump? That sounds like a bug that needs fixing anyway

spokje’s picture

Absolutely, makes sense to me.

However I would like to bump the version later on (not necessarily in this issue), because, at least to me, this issue shows the new version adds test coverage of our tests that is/was at least needed in one case.

Technically we don't need to bump the version, since the test in this issue in its new incarnation will pass in the current PHPUnit version as well.

But I think we want this extra test coverage for our tests, so we need a bump.

mondrake’s picture

Sure. In any case IMHO it’s good practice to keep these tools updated. We will need this for working on PHPUnit bump to 11 anyway.

spokje’s picture

Thanks @mondrake, per usual we agree :)

spokje’s picture

Title: [PP-1] phpunit/phpunit:10.5.30 Introduces (for Drupal) breaking changes » phpunit/phpunit:10.5.30 Introduces (for Drupal) breaking changes
Issue summary: View changes

Updated title and IS to show we want to bump the version of PHPUnit in another issue.

Needs work for removing the version bump in this issue

catch’s picture

Title: phpunit/phpunit:10.5.30 Introduces (for Drupal) breaking changes » AssetResolverTest should use ::willReturnMap() for mocking
Status: Needs work » Needs review

Rebased so the MR only includes the fix to the tests and also re-titling for the specific bug fix here.

spokje’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Thanks @catch, code changes without the version bump, so RTBC.

Tagging for followup about version bumping of PHPUnit.

  • longwave committed fd1c79fa on 10.3.x
    Issue #3468781 by Spokje, catch, mondrake: AssetResolverTest should use...

  • longwave committed 2f5393f8 on 10.4.x
    Issue #3468781 by Spokje, catch, mondrake: AssetResolverTest should use...

  • longwave committed 8d9b6739 on 11.0.x
    Issue #3468781 by Spokje, catch, mondrake: AssetResolverTest should use...

  • longwave committed 239e6f63 on 11.x
    Issue #3468781 by Spokje, catch, mondrake: AssetResolverTest should use...
longwave’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

Committed and pushed 239e6f638d to 11.x and 8d9b67396d to 11.0.x and 2f5393f842 to 10.4.x and fd1c79fa51 to 10.3.x. Thanks!

Looks like PHPUnit will be bumped in #3468502: sebastianbergmann/comparator:5.0.2 Introduces (for Drupal) breaking changes, removing tag.

Status: Fixed » Closed (fixed)

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