Opened 4 weeks ago
Last modified 45 hours ago
#66073 new task (blessed)
Tests: Let WP_Test_REST_Controller_Testcase reflect read-only routes
| Reported by: | lancewillett | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | Build/Test Tools | Version: | |
| Severity: | normal | Keywords: | dev-feedback has-patch has-unit-tests |
| Cc: | Focuses: | tests |
Description
Follow-up to #40538, splitting out the structural half so it can land on its own.
The problem
WP_Test_REST_Controller_Testcase declares nine abstract methods (tests/phpunit/includes/testcase-rest-controller.php, lines 24–40 on trunk today):
test_register_routes() test_context_param() test_get_items() test_get_item() test_create_item() test_update_item() test_delete_item() test_prepare_item() test_get_item_schema()
Every controller test must implement all nine. A read-only controller has no create, update or delete route, so those three methods have nothing to assert. Since [53921] and [54058] the accepted way out has been @doesNotPerformAssertions, with the note that it "can be reconsidered in the future".
That has not stayed flat. Trunk today carries 101 @doesNotPerformAssertions annotations in 26 files, and 98 of them are REST controller tests. Twenty files sit under tests/phpunit/tests/rest-api/, plus three under fonts/font-library/, two under icons/, and one unrelated case in user/passwordHash.php. Each new read-only controller inherits the pattern on the day it is written.
Options
- Default implementations that assert the negative. Make the six operation methods non-abstract, with a default that dispatches the operation against the controller's collection and item routes and asserts it is rejected (
rest_no_route, or the controller's documented error). A controller that supports the operation overrides the method as today. This turns 98 no-ops into real assertions with one change and no per-file work, and it stops the count growing. - A read-only base class. A second testcase with the read-side methods only, and controllers pick the one that fits. Cleaner signature, but every existing read-only controller test has to move, and nothing stops a future controller extending the wrong one.
- Leave the abstract alone and keep annotating. The status quo.
Option 1 seems the best trade, but the default needs care: some controllers register a route and reject the method with 403 or 405 rather than rest_no_route, and the schema for a POST body differs per controller. A first patch could dispatch and accept any error response, then tighten.
Once the base class change lands, the remaining annotations outside the abstract methods are a per-case read, which stays on #40538.
Change History (12)
#2
@
10 days ago
Thanks @sukhendu2002—yes, a small prototype with representative controllers sounds like the right first step. Please go ahead.
Two corrections to my original proposal:
- Existing no-op overrides would need removing.
- Accepting any error would be too weak.
We should assert the specific expected status and error code, using valid fixtures and appropriate permissions.
Let’s start with a conventional read-only controller and the block renderer, keeping its supported POST behavior covered. The icons tests offer an existing negative-request pattern, though checking rest_no_route alongside the status would make that stronger.
Some empty methods also defer to more specific tests elsewhere; those need separate treatment. Let’s use the prototype to decide whether shared defaults simplify this before changing the base class broadly.
This ticket was mentioned in PR #13817 on WordPress/wordpress-develop by @shailu25.
6 days ago
#3
- Keywords has-patch has-unit-tests added; needs-patch removed
Trac ticket: #66073
What
Prototype for #66073 that replaces empty @doesNotPerformAssertions create/update/delete (and block-renderer get_items) stubs with real negative assertions on two representative controllers:
WP_REST_Menu_Locations_Controller(conventional read-only)WP_REST_Block_Renderer_Controller(read-like POST on the item route)
Each unsupported request asserts rest_no_route with status 404, using valid fixtures and authenticated users.
Why
Read-only REST controller tests currently implement abstract create/update/delete methods as empty no-ops. That hides whether unsupported operations are actually rejected, and the annotation count keeps growing.
This small prototype follows committer feedback: prove specific status + error-code assertions on representative controllers before changing WP_Test_REST_Controller_Testcase broadly.
How
- Menu locations:
POSTcollection,POSTitem, andDELETEitem →rest_no_route/404 - Block renderer:
POST/GETcollection →rest_no_route/404(no collection route)PUT/DELETEitem →rest_no_route/404- Item-level
POSTleft covered by existingtest_get_item_post_request()
- Left
prepare_item()empty (no HTTP surface; separate treatment as discussed on the ticket) - Did not change the abstract base class yet
Testing instructions
- Start the local WordPress develop environment.
- Run:
npm run test:php -- --filter 'Tests_REST_WpRestMenuLocationsController|REST_Block_Renderer_Controller_Test'
Use of AI tools
Cursor
@lancewillett commented on PR #13817:
6 days ago
#6
PR # 13817 landed with https://core.trac.wordpress.org/changeset/64004
This ticket was mentioned in PR #13949 on WordPress/wordpress-develop by @shailu25.
2 days ago
#7
Trac ticket: #66073
What
Follow-up for #66073 that replaces empty @doesNotPerformAssertions get/create/update/delete stubs in WP_REST_Block_Directory_Controller_Test with real negative assertions.
Each unsupported request asserts rest_no_route with status 404, using an authenticated administrator and a valid-looking item fixture (guidepost).
Updated file: tests/phpunit/tests/rest-api/rest-block-directory-controller.php
Why
The block directory controller only registers GET /wp/v2/block-directory/search. Empty abstract-method stubs hide whether unsupported operations are actually rejected, and the annotation count keeps growing.
This continues the agreed approach from [64004] / PR #13817 on another conventional read-only controller, before any broader WP_Test_REST_Controller_Testcase change.
How
GET /wp/v2/block-directory/guidepost→rest_no_route/404(no item route)POST /wp/v2/block-directory/search→rest_no_route/404(search is GET-only)POST /wp/v2/block-directory/guidepost→rest_no_route/404DELETE /wp/v2/block-directory/guidepost→rest_no_route/404- Left
prepare_item()unchanged (already has real coverage) - Did not change the abstract base class yet
Testing instructions
- Start the local WordPress develop environment.
- Run:
npm run test:php -- --filter WP_REST_Block_Directory_Controller_Test
Use of AI tools
Cursor
This ticket was mentioned in PR #13950 on WordPress/wordpress-develop by @shailu25.
2 days ago
#8
Trac ticket: #66073
What
Follow-up for #66073 that replaces empty @doesNotPerformAssertions get/create/update/delete stubs in WP_REST_Pattern_Directory_Controller_Test with real negative assertions.
Each unsupported request asserts rest_no_route with status 404, using an authenticated contributor and a valid-looking item fixture (1).
Updated file: tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php
Why
The pattern directory controller only registers GET /wp/v2/pattern-directory/patterns. Empty abstract-method stubs hide whether unsupported operations are actually rejected, and the annotation count keeps growing.
How
GET /wp/v2/pattern-directory/patterns/1→rest_no_route/404(no item route)POST /wp/v2/pattern-directory/patterns→rest_no_route/404(collection is GET-only)POST /wp/v2/pattern-directory/patterns/1→rest_no_route/404DELETE /wp/v2/pattern-directory/patterns/1→rest_no_route/404- Left
prepare_item()unchanged (already has real coverage) - Did not change the abstract base class yet
Testing instructions
- Start the local WordPress develop environment.
- Run:
npm run test:php -- --filter WP_REST_Pattern_Directory_Controller_Test
Use of AI Tools
- Cursor
@lancewillett commented on PR #13949:
45 hours ago
#10
PR #13949 landed in https://core.trac.wordpress.org/changeset/64075
@lancewillett commented on PR #13950:
45 hours ago
#12
PR #13950 landed in https://core.trac.wordpress.org/changeset/64076
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
I checked the latest trunk: the nine methods are still abstract, and read-only controller tests still use @doesNotPerformAssertions for unsupported operations. I agree these should become meaningful assertions.
Before changing the base class, I think we need to decide how each test supplies its concrete routes, a valid request, and the expected rejection. Accepting any error could pass for the wrong reason—for example, a permissions failure. POST is not always a create operation, either: block rendering uses POST for a read-like request.
I’d suggest trying an explicit route/request fixture with a few representative controllers first, then making the three base methods concrete and removing the existing no-op overrides. Otherwise, those overrides would continue to shadow the new defaults.
Does that sound like a reasonable first step? Open to other approaches.