WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 3 weeks ago

#54420 reopened task (blessed)

Tests: Mock REST API remote requests

Reported by: hellofromTonya Owned by: hellofromTonya
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
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 (19)

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


4 weeks ago

#2 @hellofromTonya
4 weeks ago

  • Description modified (diff)

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


4 weeks ago

  • 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

#4 @prbot
4 weeks ago

hellofromtonya commented on PR #1868:

@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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

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

Updating keywords as ready patches were committed.

#10 follow-up: @SergeyBiryukov
4 weeks 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
4 weeks 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.


4 weeks ago

  • 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
4 weeks 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
4 weeks 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
4 weeks 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
3 weeks 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
3 weeks 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.


3 weeks ago

Note: See TracTickets for help on using tickets.