Opened 3 years ago
Closed 3 years ago
#54662 closed defect (bug) (fixed)
The `test_get_terms_post_args_paging()` test performs no assertions
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | REST API | Keywords: | |
Focuses: | Cc: |
Description
The WP_Test_REST_Tags_Controller::test_get_terms_post_args_paging()
test is faulty and performs no assertions. The response from both of the calls to WP_REST_Request
are empty, therefore nothing is iterated and the calls to $this->assertSame()
within both of the foreach()
loops are never called.
Ref: https://github.com/WordPress/wordpress-develop/runs/4574808918?check_suite_focus=true#step:20:329
Needs investigating.
Attachments (1)
Change History (7)
This ticket was mentioned in Slack in #hosting-community by costdev. View the logs.
3 years ago
#3
follow-up:
↓ 5
@
3 years ago
- Keywords reporter-feedback added
- Milestone changed from Awaiting Review to 6.1
@johnregan3 Thanks for the ping!
I believe to make WP_Test_REST_Tags_Controller::test_get_terms_post_args_paging()
perform assertions:
- The only necessary fix is:
// Line 57
// Change
$tag_ids[] = $factory->tag->create(
// To
self::$tag_ids[] = $factory->tag->create(
- Additionally, using
$this->assertNotEmpty( $tags )
before eachforeach
loop would stabilise the tests by ensuring they fail if the response returns empty.
@johnbillion What is the scope of this ticket? Is it to make the minimum number of changes to ensure that this test method always performs assertions, or is there scope for improving this test method at the same time?
If so:
- While 54662.diff improves the tests, it doesn't verify that the
orderby
argument was successful. - The test method, and patch, lacks the
message
argument, which is required for test methods with multiple assertions per Writing PHPUnit Tests - Using Assertions. - I believe this test method only needs two assertions:
$this->assertNotEmpty()
to make sure something was returned, and$this->assertSameSetsWithIndex()
to make sure the results for the given page are as expected. The rest can be handled with an additional fixture,$tag_names
, that can be set up before the class, and a data provider that allows setting thepage
andper_page
arguments for more thorough, but stable, testing. - I can attach a patch that demonstrates these improvements if this ticket has the scope for it. Otherwise, the two changes noted in the first section should be enough to ensure the method performs assertions.
P.S. Milestoning this for 6.1 as it's easily possible to resolve this during the current cycle.
#5
in reply to:
↑ 3
@
3 years ago
Replying to costdev:
I believe to make
WP_Test_REST_Tags_Controller::test_get_terms_post_args_paging()
perform assertions:
- The only necessary fix is:
// Line 57 // Change $tag_ids[] = $factory->tag->create( // To self::$tag_ids[] = $factory->tag->create(
- Additionally, using
$this->assertNotEmpty( $tags )
before eachforeach
loop would stabilise the tests by ensuring they fail if the response returns empty.
Good catch! This needed to be fixed either way and should now be resolved in [53909].
Keeping the ticket open for now for any additional improvements.
New Tests.