Follow up to #3594028: PHPUnit 13.2 - Replace expectExceptionMessage() with expectExceptionMessageIs*()

Problem/Motivation

PHPUnit 13.2 deprecates TestCase::expectExceptionMessage() in favor of TestCase::expectExceptionMessageIs() (stricter) or TestCase::expectExceptionMessageIsOrContains() (equivalent to TestCase::expectExceptionMessage()).

See Improved API for exception message expectations

Proposed resolution

In #3594028: PHPUnit 13.2 - Replace expectExceptionMessage() with expectExceptionMessageIs*() we replaced instances of TestCase::expectExceptionMessage() in core with a forward-compatibility implementation of TestCase::expectExceptionMessageIs().

That is OK on PHPUnit 12, but for PHPUnit 13 there are a number of cases where that is too strict and we should either make the expected message exact, of use the more permissive TestCase::expectExceptionMessageIsOrContains().

Exclude tests from the unit-component test suite and for tests extending directly from TestCase for the moment.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3611073

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’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

mondrake opened merge request !16332

mondrake opened merge request !16333

mondrake closed merge request !16332

mondrake’s picture

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

quietone commented
Version: 11.x-dev » main

Surely this should be on 'main', and the MR is against 'main.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#7 yes, and that’s the reason I completely messed up the first MR. I cloned the issue from the parent and did not fix the version field.

dcam’s picture

dcam commented

To test this MR I upgraded PHPUnit to 13.2 on my local environment, removed the expectExceptionMessageIs() and expectExceptionMessageIs() overrides from DrupalTestCaseTrait, and with the main branch checked out I executed all of the tests changed by the MR. They failed with 84 total failures all of which were related to incorrect exception messages. After applying the MR they passed with no failures.

The only aberrant test was Drupal\Tests\Core\Test\TestDiscoveryTest. That test did not fail on the main branch. But those two edited lines did fail after applying the MR. The same thing happened on multiple test runs. But all of the tests passed on GitLab CI. I haven't been able to figure out the cause for this discrepancy. Do you mind trying to replicate my results? Here's my test output:

There were 2 failures:

1) Drupal\Tests\Core\Test\TestDiscoveryTest::testTestInfoParserMissingGroup
Failed asserting that exception message 'Missing group metadata in test Drupal\Tests\Core\Foo\MissingAttributesTest::testNoMetadata' is 'Missing group metadata in test Drupal\Tests\Core\Foo\MissingAttributesTest'.

2) Drupal\Tests\Core\Test\TestDiscoveryTest::testTestInfoParserMissingGroupWithDataProvider
Failed asserting that exception message 'Missing group metadata in test Drupal\Tests\Core\Foo\MissingAttributesWithDataProviderTest::testNoGroupMetadata#Test#1' is 'Missing group metadata in test Drupal\Tests\Core\Foo\MissingAttributesWithDataProviderTest'.

For what it's worth, all of the changes look OK to me. I didn't see anything to comment about.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#9 thank you. Yeah the changes in that test seem wrong, at least for PHPUnit 12. Reverted here. Let's check again on PHPUnit 13 when the supporting MR is rebased after this one merged.

dcam’s picture

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

Great, thank you for double-checking it. Now all of the tests pass on the MR. This looks good to me.

  • catch committed d992d7e3 on main 
    task: #3611073 PHPUnit 13.2 - Fine tune usage of...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to main, thanks!

We should probably backport this to 11.x, there's one commit conflict though.

mondrake opened merge request !16489

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Patch (to be ported) » Needs review

Opened backport MR

dcam’s picture

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

The 11.x change are identical to what went into main with the only difference being the omission of StaticMapTest::testWithNullSourceWithEmptyStringMapping() which doesn't have the same exception.

Of the two test failures, one is that Settings Tray test that fails all the time. The other is one of the performance tests, which doesn't make sense. It hasn't been modified by this MR. I'm not aware of it being a known intermittent failure, but I'm guessing this is a random error.

  • catch committed f7660926 on 11.x 
    task: #3611073 PHPUnit 13.2 - Fine tune usage of...
catch’s picture

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

The settings tray test is head breakage, seems like an intermittent test that is now failing nearly 100% of the time.

Committed/pushed 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.