Build/Test Tools: Replace some instances of `assertEquals()`. #1768

costdev · 2021-10-19T04:49:24Z

This PR replaces assertEquals() with more appropriate, stricter assertions where possible and without making any changes to source.

For easier reviewing, commits have been separated based on the replacement assertion and any additional changes required to implement the stricter assertion.

Trac ticket: https://core.trac.wordpress.org/ticket/60706
Trac ticket: https://core.trac.wordpress.org/ticket/59655
Trac ticket: https://core.trac.wordpress.org/ticket/55654
Trac ticket: https://core.trac.wordpress.org/ticket/54726

…rmance.

…t()`.

This new assertion (introduced in this PR) is unnecessary. `assertArrayEquals()` does more testing and has the same result.

…dex()`.

desrosj

I've separated out a subset of these changes in #3413. These are the more straightforward ones where types are more apparent and don't require much digging. Also looking for clarification on a few things.

tests/phpunit/includes/abstract-testcase.php

desrosj · 2022-10-06T18:48:50Z

tests/phpunit/includes/abstract-testcase.php

@@ -1101,6 +1101,31 @@ public function assertQueryTrue( ...$prop ) {
 		}
 	}

+	/**


Could you explain why this method is required? What does this accomplish that native PHPUnit assertions do not? And why is assertEquals() OK in this context?

Functionally, nothing. However, every release, we sift through potentially hundreds of assertEquals() to use stricter assertions instead. By having a clearly named wrapper, we can reduce workload each release. In addition, assertSimilarObject() makes it clear what is being tested for those reading a test method, whereas assertEquals() is just too ambiguous.

While this isn't required, there was some support for implementing this wrapper method to make life easier going forward. I'll drop a link to this in a bit.

In terms of why assertEquals() is appropriate here, assertSame() can't be used for objects, as their IDs are different and the assertion will always fail. assertObjectEquals() is only for value objects.

See this comment and Sergey's reply.

The idea is that having the wrapper method would allow for the possibility of banning assertEquals() in tests/phpunit/tests/, to encourage stricter testing, as well as reducing our workload with each release (such as this PR)

I like the idea of having a linting check that fails if an occurrences of assertEquals() are found in the tests. That's along what I had in mind.

It looks like there is assertEqualsCanonicalizing() in PHPUnit (docs). But I'm not sure if this will work for all scenarios.

Could you take a look at that?

There also may be shortcomings of that method that are not immediately apparent. So not saying we should switch these now without further discussion.

desrosj · 2022-10-06T19:25:38Z

tests/phpunit/tests/comment.php

@@ -135,7 +135,7 @@ public function test_wp_update_comment_updates_user_id() {
 		);

 		$comment = get_comment( $comment_id );
-		$this->assertEquals( 1, $comment->user_id );
+		$this->assertSame( '1', $comment->user_id );


Want to look at this one more. The property is documented as a string, but is set to a 0 integer by default.

desrosj · 2022-10-06T19:32:54Z

tests/phpunit/tests/term.php

@@ -262,7 +262,7 @@ public function test_wp_set_term_objects_finds_term_name_with_special_characters
 		$post_id  = self::$post_ids[0];
 		$expected = wp_set_object_terms( $post_id, $name, 'category', false );


This test is weird. It's setting expected and actual to the exact same thing. Not sure why.

Hmm... that is weird.

Ah!

wp_set_object_terms() creates a new term when an existing one isn't found. It then returns an array of affected term IDs.

$expected = wp_set_object_terms( ... ) should create the term, and $actual = wp_set_object_terms( ... ) should find the existing term, with both returning an array of the same term.

If $expected !== $actual, then $actual = wp_set_object_terms( ... ) didn't find the existing term, and instead created a new one -> Failure.

desrosj · 2022-10-07T00:41:04Z

tests/phpunit/tests/customize/selective-refresh-ajax.php

@@ -188,8 +188,6 @@ public function test_handle_render_partials_request_for_non_rendering_partial()
 			)
 		);

-		$count_customize_render_partials_before = has_action( 'customize_render_partials_before' );
-		$count_customize_render_partials_after  = has_action( 'customize_render_partials_after' );


I think these tests adding 1 were working, but not checking what was originally intended. I think these changes are correct. But wanted to note what was going on for future reference.

By default, Core does not attach anything to customize_render_partials_before or customize_render_partials_after. So has_action() was returning false. Performing false + 1 below changed the value to 1, and then it was being loosely compared to true (and passing).

costdev marked this pull request as ready for review October 19, 2021 04:58

costdev changed the base branch from master to trunk November 28, 2021 01:06

costdev force-pushed the #53364 branch 3 times, most recently from 8c9ce7d to 118e6d5 Compare November 28, 2021 01:18

costdev force-pushed the #53364 branch 2 times, most recently from b71a904 to b9b3068 Compare December 12, 2021 10:30

costdev changed the title ~~Tests: Replace some instances of assertEquals() with assertSame().~~ Build/Test Tools: Replace some instances of assertEquals(). Dec 12, 2021

costdev marked this pull request as draft December 15, 2021 01:20

costdev force-pushed the #53364 branch 3 times, most recently from 084e6b7 to 5474772 Compare December 20, 2021 04:25

costdev force-pushed the #53364 branch 10 times, most recently from 95f348f to abfa900 Compare April 22, 2022 08:27

hellofromtonya self-requested a review May 2, 2022 15:21

costdev added 7 commits September 27, 2022 00:31

With assertSame().

416fa0c

With assertSame() + make $expected string.

48fd69c

With assertSame() + make $expected int.

747daa4

With assertSame() + make $expected int + cast parameter for perfo…

883060c

…rmance.

With assertSame() + make $expected string or int conditionally.

1c50642

With assertSame() + make $expected float.

5b924d6

With assertSame() + add $expected argument to data provider.

06e15df

costdev added 17 commits September 27, 2022 00:31

With assertSame() + single/multisite condition.

870cada

With assertTrue().

26fd619

With assertFalse().

203b64c

With assertNull().

e554be3

With assertSameSetsWithIndex().

0fabb2b

With assertSameSetsWithIndex() + make $expected string.

19ded55

With assertEqualSetsWithIndex().

36533d2

With assertEqualSets().

d793bd2

Add assertSimilarObject().

988354b

Add assertArrayEqualsWithObject().

a238639

With assertSimilarObject().

334f316

With assertArrayEqualsWithObject().

188d3cf

Replace use of assertArrayEqualsWithObject with `assertSimilarObjec…

35767c9

…t()`.

Replace use of assertArrayEqualsWithObject with assertEqualSets().

a11edac

Remove assertArrayEqualsWithObject().

c3d9f93

This new assertion (introduced in this PR) is unnecessary. `assertArrayEquals()` does more testing and has the same result.

Replace erroneous assertSimilarObject() with assertEqualSets().

472f974

Replace erroneous assertSimilarObject() with `assertEqualSetsWithIn…

c50e5ef

…dex()`.

costdev force-pushed the #53364 branch from abfa900 to c50e5ef Compare September 27, 2022 00:19

desrosj added a commit to desrosj/wordpress-develop that referenced this pull request Oct 6, 2022

Test a subset of WordPress#1768.

fe56223

desrosj reviewed Oct 6, 2022

View reviewed changes

desrosj reviewed Oct 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build/Test Tools: Replace some instances of `assertEquals()`. #1768

Build/Test Tools: Replace some instances of `assertEquals()`. #1768

costdev commented Oct 19, 2021 •

edited by SergeyBiryukov

desrosj left a comment

desrosj Oct 6, 2022

costdev Oct 6, 2022

costdev Oct 6, 2022 •

edited

desrosj Oct 7, 2022

desrosj Oct 7, 2022

desrosj Oct 6, 2022

desrosj Oct 6, 2022

costdev Oct 6, 2022

costdev Mar 3, 2023

desrosj Oct 7, 2022

@@ @@ -1101,6 +1101,31 @@ public function assertQueryTrue( ...$prop ) { @@
               		}
               	}
+              	/**

		@@ -262,7 +262,7 @@ public function test_wp_set_term_objects_finds_term_name_with_special_characters
		$post_id = self::$post_ids[0];
		$expected = wp_set_object_terms( $post_id, $name, 'category', false );

Build/Test Tools: Replace some instances of assertEquals(). #1768

Are you sure you want to change the base?

Build/Test Tools: Replace some instances of assertEquals(). #1768

Conversation

costdev commented Oct 19, 2021 • edited by SergeyBiryukov

desrosj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev Oct 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Build/Test Tools: Replace some instances of `assertEquals()`. #1768

Build/Test Tools: Replace some instances of `assertEquals()`. #1768

costdev commented Oct 19, 2021 •

edited by SergeyBiryukov

costdev Oct 6, 2022 •

edited