Undocumented backwards compatibility break with assertEqualsCanonicalizing

[kristiaan-factorial]

Q	A
PHPUnit version	10.5.35
PHP version	8.3.10
Installation Method	Composer

Summary

Before, assertEqualsCanonicalizing would succeed on the following code:

    $expected = ['access content overview', 'update content'];

    // This would come from a system, but for brevity's sake I inlined it.
    $actual = ['update content', 'unwanted', 'access content overview'];
    unset($actual[1]);
    
    $this->assertEqualsCanonicalizing($expected, $actual);

This was great when we were, for instance, comparing a list of permission names in Drupal. We don't care which numerical keys they have, we only care about all of the names being there. Now, it chokes on the numerical keys being different, when they don't neatly increment one after another.

Current behavior

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'access content overview'
-    1 => 'update content'
+    0 => 'update content'
+    2 => 'access content overview'
 )

How to reproduce

Try the code above

Expected behavior

That my tests would keep going green like they used to, rather than fail after updating phpunit and not seeing anything about this in the release notes :)

[kristiaan-factorial]

Seems to be fallout from the fix for this issue: #5019

[olivier-zenchef]

The core issue seems to be a change to fix sebastianbergmann/comparator#112 in release https://github.com/sebastianbergmann/comparator/releases/tag/5.0.2
Wether you agree or not with the fix, it also breaks when comparing numeric keys:

    public function test_comparator(): void
    {
        $this->assertEqualsCanonicalizing(
            [
                0 => 1,
                1 => 2,
                2 => 3,
            ],[
                0 => 1,
                1 => 2,
                3 => 3,
            ]
        );
    }

will fail with:

  Failed asserting that two arrays are equal.
   Array (
       0 => 1
       1 => 2
  -    2 => 3
  +    3 => 3
   )

Which is a breaking change. It kind of defeat the purpose of the Canonicalizing

[mgleska]

@olivier-zenchef
Look at definition of assertEqualsCanonicalizing(): https://docs.phpunit.de/en/10.5/assertions.html#assertequalscanonicalizing
We see there: For instance, when the two variables $expected and $actual are arrays, then these arrays are sorted before they are compared.

The main question is: what is an "array"?

"PHP array" is not an "array" known from other languages.
"PHP array" is an "array" only if its keys consist of consecutive numbers from 0 to count($array)-1.
"PHP array" with non-number keys or numbers keys but not consecutive is a structure known as "HashMap".
Unfortunately, PHP developers decided to name "HashMap" as "array" and "array" as "list" - see: https://www.php.net/manual/en/function.array-is-list.php

In your example actual value is [0 => 1, 1 => 2, 3 => 3,], so it is HashMap (not an array) and should not be sorted before comparison with expected.

Of course @sebastianbergmann may change assertEqualsCanonicalizing() definition to something like this:
For instance, when the two variables $expected and $actual are arrays with number keys (consecutive or not), then these arrays are sorted before they are compared.
Then [0 => 1, 1 => 2, 2 => 3,] and [0 => 1, 1 => 2, 3 => 3,] are equal in canonicalized form.

[olivier-zenchef]

@mgleska one usage of assertEqualsCanonicalizing($array1, $array2) is to compare 2 non-associative arrays in your test.
It's a shortcut to avoid manual sort + array_values

My point is that it's a breaking change because a key behaviour changed without a major step in version.

Our tests are roughly:

$expectation = [1, 2, 3];
$startingData = [1, 2, 3, 4, 5];

// Will produce [0 => 1, 1 => 2, 3 => 3] because possibility 2 was culled
$result = $myService->computeSomethingByCullingAnArray($startingData);

// Will now fail
$this->assertEqualsCanonicalizing($result, $expected);

During the computeSomethingByCullingAnArray(), there are some manipulations that unset some keys.
Those array are massive, hence why we don't copy them and rather cull them little by little.
During the whole process, we only manipulate non-associative array so we do not care for keys.

Now we want to update our phpunit version but it breaks several key tests about our key calculations.

• either we change a lot of our code just to accommodate what should be a non-breaking update of a test suite
• either we impact our test suite speed by adding a lot of array_values and sorting inside a small helper

In both cases this is not what we expected when upgrading a minor version.

[kristiaan-factorial]

To make it clear, there are two points being argued for here:

1. A minor version update broke our tests, this is undeniable and shouldn't have happened outside of a major release
2. The old behavior is preferred over the new (at least for numerical arrays), because the new makes tests overly verbose and beats the purpose of canonicalizing

We can still discuss what to do about point 2 and whether we want to revert the behavior to the old one for numerical arrays, even if their keys are out of order. But point 1 should not be up for discussion as that's simple fact.

[olivier-zenchef]

For the record and google searchers:
Temporary fix is to block phpunit/phpunit version to 10.5.29 and also the faulty sub-lib sebastian/comparator to 5.0.1:

composer require phpunit/phpunit:10.5.29 --dev
composer require sebastian/comparator:5.0.1 --dev

You have to do both as phpunit require comparator from 5.0.2 starting tag 10.5.30.

[sebastianbergmann]

sebastian/comparator 5.0.3 and sebastian/comparator 6.1.1 have been released and revert sebastianbergmann/comparator#113.

[olivier-zenchef]

@sebastianbergmann thanks for listening to our feedback and for the quick fix 🙏
We don't have the issue anymore all the way up to the latest 10.5.37