Support indexed arrays in ArraySubset

[pfrenssen]

This fixes #3101 and earlier efforts #2781 and #2069.

I have added support for indexed arrays, and it is now possible to correctly check multidimensional arrays. It allows to match indexed arrays that are out of order, so for example if you have an array [0, 1, 2, 3] you can assert that [3, 1] is a valid subset.

Because of this out-of-order matching there is a known limitation, the code loops over the data set and checks every data point and returns on the first match. This means it will generate a false positive if multiple identical values are present in the subset. For example if an array [0, 1, 2, 3] is given and you assert the subset [0, 1, 1, 2] then it will return TRUE even though 1 is present only once in the data set. I wasn't sure if the added complexity for supporting this edge case is worth it.

This is my first contribution to PHPUnit and I am not sure on a few points:

1. I would like to include this in both PHPUnit 6 and 7, but I don't know the procedure to follow. This PR is rolled against the current master. Can this be backported or am I supposed to roll this for PHPUnit 6 first?
2. I am heavily relying on anonymous functions, I don't know if this fits the code style of PHPUnit, or would it be prefered to rework this using private methods?

[codecov-io]

Codecov Report

Merging #3161 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #3161      +/-   ##
===========================================
+ Coverage     81.32%   81.4%   +0.07%     
- Complexity     3384    3406      +22     
===========================================
  Files           137     137              
  Lines          8966    9003      +37     
===========================================
+ Hits           7292    7329      +37     
  Misses         1674    1674

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/Constraint/ArraySubset.php	98.55% <100%> (+1.67%)	33 <23> (+22)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 418c58a...e9ea3d0. Read the comment docs.

[sebastianbergmann]

I prefer private methods over anonymous functions.

[sebastianbergmann]

If this is a bug fix and the respective bug is in 6.5 then this should go into 6.5. It will then be merged from there to 7.2 and master.

If this is an enhancement it will go into master only.

[pfrenssen]

Thanks for the quick reply! I will refactor this using private methods.

Regarding or not whether this is a bug fix, I think this can be debatable. I personally think of this as a bug since the current documentation does not mention that it is only designed to work with associative arrays:

/**
 * Asserts that an array has a specified subset.
 *
 * @param array|ArrayAccess $subset
 * @param array|ArrayAccess $array
 * @param bool              $strict  Check for object identity
 * @param string            $message
 *
 * @throws Exception
 */
function assertArraySubset($subset, $array, bool $strict = false, string $message = ''): void;

When I read this documentation, then I would expect the following to return TRUE, but it doesn't. It feels a lot like a bug:

$this->assertArraySubset( ['b', 'c'], ['a', 'b', 'c', 'd'] );

However after I started working on this I read in #2069 (comment) that @marcioAlmada was only targeting associative arrays in the original implementation. So I guess from his point of view this is a new feature.

[sebastianbergmann]

I think it should go into master then. Unless it changes/breaks current behaviour. Then it would have to wait for PHPUnit 8.

[pfrenssen]

OK! To the best of my knowledge there should not be a B/C break. I took care not to break the existing implementation for associative arrays, and have not touched the original tests, they are still passing.

If people are using the current implementation for indexed arrays then there might be some unexpected failures appearing, but this will actually be because bugs in their code are now being correctly detected.

For example, this will previously have returned FALSE, but now it will (correctly) return TRUE:

$this->assertArraySubset( ['b', 'c'], ['a', 'b', 'c', 'd'] );

Is it the policy of PHPUnit to never cause any existing test to change output, even if the behaviour relied on is objectively broken?

[pfrenssen]

Thanks!

[chx]

This is weird. Are you sure this shouldn't be asort? The sort just below sorts by value and this sorts by key. asort would be more analogous.

[pfrenssen]

The goal is to be able to compare the two arrays, and so both ksort() and asort() do the trick for this use case. You do have a point that it would be more consistent using asort().

[chx]

Got it, same arrays sort to the same regardless you sort them by keys or values but for numeric arrays you wanted [1,2] and [2,1] to be the same.

[taylorotwell]

This was a breaking change. Broke the existing Laravel framework tests.

[sebastianbergmann]

@taylorotwell Please be open a ticket for this breaking change (and be more specific than "Broke the existing Laravel framework tests"). Thanks!

[pfrenssen]

@taylorotwell thanks for the report. Can you check if the tests still fail if you run them using PR #3186?

In that PR support is added for checking subsets that contain duplicate values. This might be the cause of the failures.