Problem/Motivation

The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Status: Active » Needs review
FileSize
1.16 KB

Kickoff patch, unsilencing the deprecation to see size of the pie.

Status: Needs review » Needs work

The last submitted patch, 2: 3126333-2.patch, failed testing. View results

longwave’s picture

assertEqualsCanonicalizing() was added in PHPUnit 7.5 so we cannot do a straight backport, we would need a forward-compatibility shim. But I am not sure it matters for such a tiny diff?

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#3118454: Drupal\KernelTests\Core\Database\SelectTest fails on postgres 10 already introduced usage of the method in D9 only. I see this is quite similar. LGTM

  • catch committed bcbb697 on 9.1.x
    Issue #3126333 by mondrake, longwave: Replace usage of the optional $...

  • catch committed 2d0a139 on 9.0.x
    Issue #3126333 by mondrake, longwave: Replace usage of the optional $...
catch’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Opened #3126695: [D8 only] Add forwards-compatibility shim for assertEqualsCanonicalizing() in phpunit 6&7 against 8.9.x - not really so that we can backport this, but because it would help contrib compatibility. However if we did do that, we could backport this, so I guess it makes sense to leave open.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

longwave’s picture

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

thanks, lgtm

xjm’s picture

8.9.x supports PHP 7.0, and therefore needs compatibility with PHPUnit 6. Are we sure we don't need a PHPUnit 6 shim for this?

I queued some tests on PHP 7.0 environments.

xjm’s picture

Looks like the PHP 7.0 tests passed, which should have used PHPUnit 6. ...Ah, and it's already provided in the PHPUnit 6 PhpunitCompatibilityTraitTest. Cool.

What on earth are those extra arguments in HEAD, the 0.0 and 10? The only reference I could find is:

assertEquals(float $expected, float $actual[, string $message = '', float $delta = 0])

Reports an error identified by $message if the absolute difference between two floats $expected and $actual is greater than $delta. If the absolute difference between two floats $expected and $actual is less than or equal to $delta, the assertion will pass.

...But that's not what we had before.

I blamed this and it was added in #1267508-67: Subselects don't work in DBTNG conditions, except when used as value for IN with the comment:

assertArraySubset() takes order (indices) into account. [My BTW turned out to be prophetic :).] However, it turns out that assertEquals has a $canonicalize parameter that sorts array before comparing them. This parameter turns this assert into a check on whether both arrays have the same values (with the same cardinality).

But that still doesn't explain what's up with these extra args or what they're providing coverage for... at the time, it might still have been SimpleTest assertions. 🤷‍♀️

Since this is a DB test, I'm going to queue tests against the various supported databases.

mondrake’s picture

#13 those are legacy PHPUnit arguments of the assertEquals method, that were used to provide a tolerance delta for float comparison ($delta) and to specify the max nesting level when comparing nested arrays/objects.

PHPUnit got rid of the catch-em-all method in favour of specialized equality assertions in https://github.com/sebastianbergmann/phpunit/issues/3341. We are catching up with that here, natively in D9 and with fc shims in D8.

We used to have an assertEquals override in Drupal (guess that's the one you found, @xjm), that was removed in D8.9, see #3082340: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation, but not backported, see comment #67 there.

  • xjm committed 0d1d15c on 8.9.x
    Issue #3126333 by longwave, mondrake, xjm, catch: Replace usage of the...

  • xjm committed 6a70c36 on 8.8.x
    Issue #3126333 by longwave, mondrake, xjm, catch: Replace usage of the...
xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks @mondrake! Mystery solved. How weird.

The tests for older databases passed, so committed #10 to 8.9.x and cherry-picked to 8.8.x.

Status: Fixed » Closed (fixed)

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