Make WordPress Core

Opened 2 years ago

Last modified 17 months ago

#54420 reopened defect (bug)

Tests: Mock REST API remote requests

Reported by: hellofromtonya's profile hellofromTonya Owned by: hellofromtonya's profile hellofromTonya
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch dev-feedback
Focuses: rest-api Cc:

Description (last modified by hellofromTonya)

Recent timeout failures (see GitHub failed job) of specific REST API tests show some tests are not mocking the remote requests. These requests can be mocked with a callback hooked into 'pre_http_request' filter.

Each test should be reviewed to determine:

  • If a remote request to a live API endpoint is being made
  • If those specific requests should/can be mocked
  • and then mocks created for each in its context of the condition under test

Ideally a mocking strategy could be created to abstract the heavy lifting and make it easier for these tests to mock the requests.

Change History (26)

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


2 years ago

#2 @hellofromTonya
2 years ago

  • Description modified (diff)

This ticket was mentioned in PR #1868 on WordPress/wordpress-develop by hellofromtonya.


2 years ago
#3

  • Keywords has-patch has-unit-tests added

Recent timeout failures (see this job) are due to remote requests not being mocked. This patch mocks the specific failing tests, though each test will need to be investigated separately for mocking strategy.

Trac ticket: https://core.trac.wordpress.org/ticket/54420

hellofromtonya commented on PR #1868:


2 years ago
#4

@TimothyBJacobs This PR mocks the 2 failing tests we saw https://wordpress.slack.com/archives/C02RQBWTW/p1636603357442100

❗ A run of the PHPUnit Tests workflow has failed for the trunk branch.

REST API: Add /wp/v2/block-navigation-areas endpoint

More details: https://github.com/WordPress/wordpress-develop/actions/runs/1447262178.

The ticket I opened takes the slow road to fix each of these as found.

Can you do a confidence check of the mocks?

#5 @hellofromTonya
2 years ago

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

In 52137:

Build/Test Tools: Mock remote request for WP_REST_Block_Directory_Controller::get_items().

Instead of hitting the live API, this commit mocks the remote request.

Follow-up to [48242].

Props hellofromTonya, noisysocks, sergeybiryukov, TimothyBlynJacobs.
Fixes #54420.

#6 @hellofromTonya
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Whoopsie, didn't mean to close it. Commit should have been See not Fixes.

#7 @hellofromTonya
2 years ago

In 52138:

Build/Test Tools: Mock remote request for unknown plugin in WP_REST_Plugins_Controller::create_item().

Instead of hitting the live API, this commit mocks the remote request when testing creating an item that's an unknown plugin.

Follow-up to [48242].

Props hellofromTonya, noisysocks, sergeybiryukov, TimothyBlynJacobs.
See #54420.

#9 @hellofromTonya
2 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

Updating keywords as ready patches were committed.

#10 follow-up: @SergeyBiryukov
2 years ago

Thanks for the ticket and the fixes!

Just noting that [52137] addresses the failure in test_get_items(), but not in test_get_items_no_results(), which also failed in the same job and apparently needs a similar fix.

Should we move the pre_http_request filter into a helper method to be reused there?

#11 in reply to: ↑ 10 @hellofromTonya
2 years ago

Replying to SergeyBiryukov:

Thanks for the ticket and the fixes!

Just noting that [52137] addresses the failure in test_get_items(), but not in test_get_items_no_results(), which also failed in the same job and apparently needs a similar fix.

Should we move the pre_http_request filter into a helper method to be reused there?

Whoops, missed that one. Thanks for the ping @SergeyBiryukov. To keep it simply, I agree with adding a helper method. Doing it now.

This ticket was mentioned in PR #1871 on WordPress/wordpress-develop by hellofromtonya.


2 years ago
#12

  • Keywords has-patch has-unit-tests added; needs-patch removed

Mocks remote request in WP_REST_Block_Directory_Controller_Test:: test_get_items_no_results().

To reduce redundant code, moves the mock to a helper method that accepts expected request with those specific args to override.

Trac ticket: https://core.trac.wordpress.org/ticket/54420

#13 follow-up: @SergeyBiryukov
2 years ago

Carrying this over from comment:37:ticket:51669:

Got this again when running multisite tests:

There was 1 error:

1) WP_REST_Plugins_Controller_Test::test_create_item
An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/forums/">support forums</a>. (WordPress could not establish a secure connection to WordPress.org. Please contact your server administrator.)

/var/www/build/wp-admin/includes/plugin-install.php:183
/var/www/build/wp-includes/rest-api/endpoints/class-wp-rest-plugins-controller.php:288
/var/www/build/wp-includes/rest-api/class-wp-rest-server.php:1139
/var/www/build/wp-includes/rest-api/class-wp-rest-server.php:985
/var/www/tests/phpunit/includes/spy-rest-server.php:57
/var/www/build/wp-includes/rest-api.php:479
/var/www/tests/phpunit/tests/rest-api/rest-plugins-controller.php:379

Apparently [49913] and [49951] didn't cover the external HTTP request in plugins_api(). Looks like the plugins_api() call in these tests is by design, so they should probably use the external-http group and the skipTestOnTimeout() method, or mock the response.

#14 in reply to: ↑ 13 @SergeyBiryukov
2 years ago

Replying to SergeyBiryukov:

Carrying this over from comment:37:ticket:51669:

On second thought, it looks like [52138] may have already addressed that :)

#15 @hellofromTonya
2 years ago

In 52146:

Build/Test Tools: Mock no results remote request in WP_REST_Block_Directory_Controller:: get_items().

  • Refactors the mock logic to a helper function for reuse in multiple tests.
  • Mocks the remote request in WP_REST_Block_Directory_Controller_Test:: test_get_items_no_results() using the mock helper.

Follow-up to [48242], [52137].

Props hellofromTonya, sergeybiryukov.
See #54420.

#17 follow-up: @hellofromTonya
2 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

Tests in Tests_Admin_IncludesTheme::test_get_theme_featured_list_api() timed out in this CI Job, i.e. Run external HTTP tests. Noting it here as another test for mocking the HTTP remote request.

#18 in reply to: ↑ 17 @SergeyBiryukov
2 years ago

Replying to hellofromTonya:

Tests in Tests_Admin_IncludesTheme::test_get_theme_featured_list_api() timed out in this CI Job, i.e. Run external HTTP tests. Noting it here as another test for mocking the HTTP remote request.

This was added in [39906] / #28121 to make sure that the list of theme features pulled from the WordPress.org API returns the expected data structure. Wouldn't mocking this response defeat the purpose of the test?

This raises an interesting point: when mocking the response, we should consider the intent of the test, and whether it was specifically written to protect from unexpected changes in the WordPress.org API response.

This may very well also apply to the tests changed in [52137] and [52138]. Note that the latter previously had a comment: // This will hit the live API. Seems worth investigating whether that live request was intentional. Perhaps a better option would be to add them to the external-http group and ignore any timeouts? See the WP_UnitTestCase_Base::skipTestOnTimeout() method and the tests using it for example.

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


2 years ago

#20 @SergeyBiryukov
2 years ago

In 52382:

Tests: Mock the HTTP request response in download_url() tests.

This aims to speed up the tests and minimize unrelated failures by avoiding an unnecessary external HTTP request, while still performing the intended functionality checks.

Update similar helpers in some other tests to use more consistent terminology.

Follow-up to [37907], [46175], [51626].

See #54420, #53363.

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


23 months ago

#22 @sumitsingh
23 months ago

  • Keywords dev-feedback added

#23 @hellofromTonya
22 months ago

  • Milestone changed from 6.0 to 6.1

With RC1 tomorrow, moving to 6.1.

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


17 months ago

#25 @audrasjb
17 months ago

As per today's bug scrub:
Unfortunately, this ticket wasn’t updated since it was moved from milestone 6.0 to 6.1. Since WP 6.1 RC1 is coming next Tuesday, let’s move it to Future Release.

Feel free tom move it to the next milestone (6.2).

#26 @hellofromTonya
17 months ago

  • Milestone changed from 6.1 to Future Release
  • Type changed from task (blessed) to defect (bug)

Moving to Future Release until there's consensus on dev feedback. Also changing the type to defect instead of blessed task.

Note: See TracTickets for help on using tickets.