WP_UnitTestCase::assertEqualSets can pass incorrectly

[dd32]

WP_UnitTestCase::assertEqualSets currently allows different sized sets to be passed, which can hide errors in unit tests.

Take for example the following unit test, I can't see a reason for why this should pass, and yet, it currently does:

$this->assertEqualSets( array( 1, 2, 3 ), array( 1, 2, 2, 3, 3, 3 ) );

There's a few options, including asserting the count is the same, however that doesn't work as something like this would then still pass:

$expected = array( 1, 2, 2 );
$actual = array( 1, 1, 2 );
$this->assertEqualSets( $expected, $actual );

The function currently only cares about the values (as it uses array_diff()), so we could simply perform a sort and then assertEquals the resulting array, that would account for the above cases.

However, we also have some tests (Terms for example) which are testing id=>slug, those tests could have the incorrect id (array key) and still pass (both now, and with the sorting option), eg:

$expected = array(
	1 => 'one',
	100 => 'one hundred'
);
$actual = array(
	50 => 'one',
	500 => 'one hundred'
);
$this->assertEqualSets( $expected, $actual );

assertEqualSets was introduced in [1127/tests].

[pento]

30522.diff​ fixes the behaviour of assertEqualSets(), and adds assertEqualSetsWithIndex(), which does the same as assertEqualSets(), but maintains index association.

Patch includes unit tests for both.

[dd32]

To be clear, the reason there's 2 functions to replace the existing function is because assertEqualSets() can't be used when you need to retain index association, my third example in the ticket should not be using assertEqualSets() as you can't effectively match the requirement for unsorted sets with an incremental numeric key, and a set using a specified key => val pairs at the same time.

This means that any tests in core which are testing id=>slug such as the getTerm tests, should use assertEqualSetsWithIndex() (which could potentially be called assertEqualAssocSets()) so that it can verify that [ 1 => 'Term1', 2 => 'Term2' ] != [ 0 => 'Term1', 1 => 'Term2' ].

[bobbingwide]

If I remember correctly, see Nov 6 slack archive, I reported one of the problems with the PHPUnit tests tests could be that assertEqualSets ignores duplicate keys so doesn't notice get_pages() returning array(3,4,5,5) when it should be array(3,4,5);

I believe there is a need to include a test that ensures the arrays are the same without involving sorting.
e.g. for a query where you want to get posts 1,4,3,2 in that order, then the expected array should be ( 1,4,3,2 ) in that order.

Question: Can't assertEquals be used for that specific test?

I received a response from @dancameron who said

I wrote the tests in r30096 for #12668. I believe assertEquals respects order and assertEqualSets does not. It's possible my tests are wrong though, even though they succeeded pre & post. Let me know what you find and I can take a look; maybe a new ticket is in order (?).

If the answer to my above Question is No, then I believe that this is the ticket that I should have raised.

[dd32]

I believe there is a need to include a test that ensures the arrays are the same without involving sorting.
e.g. for a query where you want to get posts 1,4,3,2 in that order, then the expected array should be ( 1,4,3,2 ) in that order.
Question: Can't assertEquals be used for that specific test?

Yes, AssertEquals should be used when both value and order of the returned data is important.
It should not be used when the order is unimportant, or potentially undefined, which is where assertEqualSet was introduced.

I reported one of the problems with the PHPUnit tests tests could be that assertEqualSets ignores duplicate keys so doesn't notice get_pages() returning array(3,4,5,5) when it should be array(3,4,5);

That's this exact bug :) I guess it got missed on Slack, or no-one created a Ticket for it.

I wrote the tests in r30096 for #12668. I believe assertEquals respects order and assertEqualSets does not

That's a correct assumption.
The tests look good to me, based on the idea that you expect the order of the comments to be returned in the specific order. However, those tests may actually fail on older MySQL, see #30478 as to why

[boonebgorges]

The fix to assertEqualSets in 30522.diff​ look good to me.

The use case for assertEqualSetsWithIndex() is pretty narrow, but it looks fine to me. It'd be swell to have a guide to all of our custom assertions, like maybe here ​https://make.wordpress.org/core/handbook/automated-testing/#writing-tests.

The tests-of-tests should probably go in tests/phpunit/tests/includes/.

This means that any tests in core which are testing id=>slug such as the getTerm tests, should use assertEqualSetsWithIndex()

Yes. And in fact, this shows a bug in one of our tests (though thankfully not get_terms() itself!). See 30522-getTerms.diff​.

[pento]

In 30687:

Unit Tests: The assertEqualSets() helper was returning true for some sets that were not equal. assertEqualSets() now behaves correctly, and the new assertEqualSetsWithIndex() helper also checks that the array indexes are the same.

Fixes #30522.

[pento]

Reopen for @boonebgorges' getTerms unit test changes.

[boonebgorges]

In 30688:

Use assertEqualSetsWithIndex() as appropriate in get_terms() tests.

See #30522.

[boonebgorges]

I've reviewed the rest of the uses of assertEqualSets() in our existing tests, and it looks like they're all good. Thanks for your help sorting this out, gentlemen.