WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30522 closed defect (bug) (fixed)

WP_UnitTestCase::assertEqualSets can pass incorrectly

Reported by: dd32 Owned by: pento
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description (last modified by 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].

Attachments (2)

30522.diff (6.3 KB) - added by pento 6 years ago.
30522-getTerms.diff (3.0 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (15)

This ticket was mentioned in Slack in #core by dd32. View the logs.


6 years ago

#2 @dd32
6 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by pento. View the logs.


6 years ago

@pento
6 years ago

#4 @pento
6 years ago

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.

#5 @pento
6 years ago

  • Keywords has-patch added

#6 @dd32
6 years ago

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' ].

#7 @bobbingwide
6 years ago

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.

Last edited 6 years ago by bobbingwide (previous) (diff)

#8 @dd32
6 years ago

  • Version set to trunk

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

#9 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 4.1

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.

#10 @pento
6 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

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.

#11 @pento
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for @boonebgorges' getTerms unit test changes.

#12 @boonebgorges
6 years ago

In 30688:

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

See #30522.

#13 @boonebgorges
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.