WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 months ago

Last modified 7 months ago

#39122 closed enhancement (fixed)

REST API: Add Term Meta Unit Test Coverage

Reported by: timmydcrawford Owned by: kadamwhite
Milestone: 4.9.9 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords:
Focuses: Cc:

Description

This is a follow up to #38989 where I had submitted a diff with some basic custom term + meta unit tests that didn't really apply to the issue at hand. @rachelbaker suggested ( https://core.trac.wordpress.org/ticket/38989#comment:4 ) that I add the tests in a new issue.

The tests assert that register_meta on terms are properly included in term responses, and casted correctly for single=>true and single=>false. Additionally the tests act as documentation of sorts on how to interact with custom meta with terms in the REST API.

Attachments (4)

add-term-meta-unit-tests.diff (2.2 KB) - added by timmydcrawford 3 years ago.
39122.diff (7.6 KB) - added by kadamwhite 12 months ago.
39122.2.diff (9.9 KB) - added by birgire 12 months ago.
39122.3.diff (3.9 KB) - added by birgire 12 months ago.

Download all attachments as: .zip

Change History (28)

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


3 years ago

#2 @kadamwhite
12 months ago

Expanded patch to cover the tags controller, and to test the new register_term_meta functions to ensure term meta can be limited to a single taxonomy's endpoint.

@kadamwhite
12 months ago

#3 @kadamwhite
12 months ago

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

In 43567:

Tests: Improve coverage for REST API term meta registration.

Introduce tests to validate that register_meta and register_term_meta work as expected in WP_REST_Terms_Controller.

props timmydcrawford.
Fixes #39122.

#4 @Kniebremser
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Does not the ticket have to be moved from "Awaiting Review" to "Milestone 5.0.0"?

@birgire
12 months ago

#5 @birgire
12 months ago

Since the ticket has been reopen, 39122.2.diff contains some suggestions:

  • Adjustments according to the WPCS.
  • Removes duplicated array key assertion: $this->assertArrayHasKey( 'meta', $data );
  • Adds @ticket annotations.
  • Use asserSame instead of assertEquals, as null, false and '' are equal under assertEquals.
  • Use assertFalse instead of assertEquals( false, ... )

Additional note: Instead of $this->assertEquals( false, isset( $meta['test_cat_meta'] ) );, one could consider assertArrayNotHasKey, but we note that it also returns true for null values. But I didn't change it in the patch to use assertArrayNotHasKey.

Last edited 12 months ago by birgire (previous) (diff)

#6 @pento
12 months ago

  • Milestone changed from Awaiting Review to 5.0

[43567] also caused changes to the REST API fixtures, they need to be regenerated and committed.

#7 follow-up: @pento
12 months ago

Nevermind, I accidentally committed the wp-api-generated.js update in [43571] 🙂.

@birgire: If you wouldn't mind refreshing your patch since [43571] auto-fixed a bunch of coding standards issues, we can get that committed.

#8 @pento
12 months ago

In 43572:

Tests: Revert wp-api-generated.js change added in [43571].

The fixtures file was accidentally included in [43571], but that caused other tests to fail.

See #39122.

@birgire
12 months ago

#9 in reply to: ↑ 7 @birgire
12 months ago

Replying to pento:

Nevermind, I accidentally committed the wp-api-generated.js update in [43571] .

@birgire: If you wouldn't mind refreshing your patch since [43571] auto-fixed a bunch of coding standards issues, we can get that committed.

yes sure, it looks like the WPCS has been fixed, so I updated the patch with 39122.3.diff.

When I was preparing the previous 39122.2.diff, I almost patched the changes to the auto generated wp-api-generated.js too :-) I suspect we will see more of such patches in the future :-)

Last edited 12 months ago by birgire (previous) (diff)

#10 @SergeyBiryukov
11 months ago

  • Milestone changed from 5.0 to 4.9.9

Moving to 4.9.9, as the tests in #44834 depend on test_tag_single term meta added in [43567].

#11 @SergeyBiryukov
11 months ago

In 43646:

Tests: Improve coverage for REST API term meta registration.

Introduce tests to validate that register_meta and register_term_meta work as expected in WP_REST_Terms_Controller.

Props timmydcrawford.
Merges [43567] to the 4.9 branch.
See #39122.

#12 follow-up: @SergeyBiryukov
11 months ago

  • Keywords has-patch commit added

39122.3.diff should be committed and backported.

#13 @pento
11 months ago

  • Milestone changed from 4.9.9 to 5.0

#14 @SergeyBiryukov
10 months ago

@pento: [43646] was committed to the 4.9 branch, to fix an issue with the tests added in [43637], which depend on the test_tag_single fixture added here.

If the ticket is set to 5.0, should [43637] and [43646] be reverted from the 4.9 branch? Or should be keep them and bring the ticket back to 4.9.9?

#15 @pento
10 months ago

  • Milestone changed from 5.0 to 4.9.9

Yeesh, Let's just keep them.

#16 in reply to: ↑ 12 @SergeyBiryukov
10 months ago

Replying to SergeyBiryukov:

39122.3.diff should be committed and backported.

Moved to a separate ticket for easier management: #45077 (as trunk is currently closed for commits).

#17 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43713:

Tests: Improve coverage for REST API term meta registration.

Introduce tests to validate that register_meta and register_term_meta work as expected in WP_REST_Terms_Controller.

Props timmydcrawford.
Merges [43567] to the 5.0 branch.
Fixes #39122.

#18 follow-up: @adamsilverstein
10 months ago

@SergeyBiryukov - in r43713 the wp-api-generated.js file is changed because of leftover meta fields set up with register_meta in the setUp function of rest-tags-controller.php. In the tearDown function we can use unregister_meta to remove these and the fixture would not need to change.

#19 in reply to: ↑ 18 @SergeyBiryukov
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to adamsilverstein:

in r43713 the wp-api-generated.js file is changed because of leftover meta fields set up with register_meta in the setUp function of rest-tags-controller.php.

Somehow I get different results when running grunt phpunit:restapi-jsclient vs. phpunit --group restapi: the latter includes these extra keys, the former does not.

Noticed after @atimmer pointed out that running grunt prerelease leads to a modified wp-api-generated.js file (in trunk and both 4.9 and 5.0 branches).

Reopening for investigation.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

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


10 months ago

#21 @pento
10 months ago

  • Keywords needs-patch added; has-patch commit removed

#22 @pento
10 months ago

@SergeyBiryukov: Are you still working on this?

#23 @pento
9 months ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

#24 @pento
7 months ago

In 44510:

Tests: Improve REST API tests for categories and tags.

Props birgire, SergeyBiryukov.
See #39122.
Fixes #45077.

Note: See TracTickets for help on using tickets.