Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#39264 closed enhancement (fixed)

Add WP-API JS Client unit tests to core

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: fixed-major
Focuses: javascript Cc:

Description

I would like to get get the QUnit tests we wrote for the WP-API Backbone JavaScript client merged into core and running as part of our build and CI process. I could really use some help getting the configuration right.

I am attaching a patch file which adds the tests to our core qunit suite. In order for the tests to run correctly they must connect to the api on a local WordPress install. In my local environment I can get this to work by loading the test page from my local dev environment. When set up correctly all the tests pass. I think we need to spin up this environment to test against as part of the build.

Attachments (14)

39264.diff (4.2 KB) - added by adamsilverstein 8 years ago.
39264.2.diff (135.1 KB) - added by adamsilverstein 8 years ago.
39264.3.diff (141.4 KB) - added by adamsilverstein 8 years ago.
39264.4.diff (15.7 KB) - added by adamsilverstein 8 years ago.
39264.5.diff (16.4 KB) - added by welcher 8 years ago.
Grunt task added
39264.6.diff (16.5 KB) - added by jnylen0 8 years ago.
Cleaned up formatting, naming, etc.
39264.7.diff (83.5 KB) - added by adamsilverstein 8 years ago.
slight cleanup for jshint
39264.8.diff (332 bytes) - added by netweb 8 years ago.
39264.9.diff (10.4 KB) - added by jnylen0 8 years ago.
Ensure that wp-api-generated.js fixtures remain the same across test runs
compare-wp-api-fixtures.js (692 bytes) - added by jnylen0 8 years ago.
Script to list diffs of generated fixture data
39264.10.diff (244.2 KB) - added by adamsilverstein 8 years ago.
39264.11.diff (48.6 KB) - added by jnylen0 8 years ago.
Remove JSON_UNESCAPED_SLASHES; make phpunit exit correctly if no config file
39264.12.diff (48.6 KB) - added by jnylen0 8 years ago.
Skip fixture file generation on PHP < 5.4; bring back JSON_UNESCAPED_SLASHES.
39264.13.diff (1.1 KB) - added by jnylen0 8 years ago.
¡Una más!

Download all attachments as: .zip

Change History (64)

#1 @jorbin
8 years ago

I chatted in person with @mboynes and he said he may have some ideas for this, so tagging him in.

#2 @adamsilverstein
8 years ago

Thanks @jorbin! Any help here appreciated. I chatted with @westonruter and he gave me the idea of adding a fixture and $.ajax shim to mix in the mocked returns. I am working on this locally and will post a patch soon if I get it working.

#3 @adamsilverstein
8 years ago

In 39264.2.diff:

  • Include mocked responses captured from actual api responses on a vanilla 4.7 install.
  • Add an ajax shim to return the mocked data for the ajax callbacks
  • All tests pass

#4 @adamsilverstein
8 years ago

  • Keywords has-patch 2nd-opinion added

#5 @adamsilverstein
8 years ago

  • Focuses javascript added

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


8 years ago

#7 @adamsilverstein
8 years ago

  • Milestone changed from Awaiting Review to 4.7.2

#8 @adamsilverstein
8 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@jesseenterprises or @jorbin appreciate any feedback/testing of 39264.2.diff !

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


8 years ago

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


8 years ago

#11 @adamsilverstein
8 years ago

A great improvement here would be automating the generation of the mocked data, ideally as a grunt task.

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


8 years ago

#13 @adamsilverstein
8 years ago

39264.3.diff First pass at adding schema data generation as part of a phpunit test.

running phpunit --group=restapi-jsclient will generate the json required for the QUnit tests in tests/qunit/data.

Next step is to switch to using the generated data and remove the mocked data.

#14 @adamsilverstein
8 years ago

39264.4.diff

  • complete construction of dynamic schema data for QUnit tests as part of a PHPUnit test
  • run phpunit --group=restapi-jsclient to run the schema tests and generate the js data file for the QUnit tests
  • run qunit to run the js client tests

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


8 years ago

#16 @adamsilverstein
8 years ago

  • Keywords has-unit-tests added

Appreciate a review of the approach here @welcher, @jorbin, @rmccue or @westonruter - I'd like to get this committed ASAP so I can add tests for other fixes I have in my commit queue.

Thanks

#17 @welcher
8 years ago

@adamsilverstein everything worked great for me - fantastic work!

I've added a patch that just removes a small line of code that was commented out. I have also added a new grunt task to automate this - please have a look and let me know if it makes sense and be sure I'm hitting all of the processes needed.

@welcher
8 years ago

Grunt task added

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


8 years ago

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


8 years ago

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


8 years ago

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


8 years ago

#22 @jnylen0
8 years ago

  • Keywords commit added; 2nd-opinion removed

@adamsilverstein let's get this committed so we can move on to adding other tests. In 39264.6.diff I didn't make any major changes, just cleaned up a bunch of formatting stuff, removed the foreach loop in the PHP tests, and made naming more consistent overall. Notably:

  • wp-api.jswp-api-generated.js
  • wp-api-2.jswp-api.js

@jnylen0
8 years ago

Cleaned up formatting, naming, etc.

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


8 years ago

#24 @adamsilverstein
8 years ago

@jnylen0 Thanks for the cleanup, naming changes and also your suggestion that we commit the generated file as well. This adds a record of changes to the data and allows the qunit tests to run without first running phpunit. It should also allow the tests to pass in travis with our existing configuration.

I submitted the patch as a PR to trigger a Travis run, if everything passes, I will commit this.

@adamsilverstein
8 years ago

slight cleanup for jshint

#25 @adamsilverstein
8 years ago

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

In 40058:

REST API: Add QUnit tests for wp-api.js and PHPUnit fixture generation.

Add QUnit tests: verify that wp-api loads correctly, verify that the expected base models and collections exist and can be instantiated, verify that collections contain the correct models, verify that expected helper functions are in place for each collection.

The QUnit tests rely on two fixture files: tests/qunit/fixtures/wp-api-generated.js contains the data response from each core endpoint and is generated by running the PHPUnit restapi-jsclient group. tests/qunit/fixtures/wp-api.js maps the generated data to endpoint routes, and overrides Backbone.ajax to mock the responses for the tests.

Add PHPUnit tests in tests/phpunit/tests/rest-api/rest-schema-setup.php. First, verify that the API returns the expected routes via server->get_routes(). Then, the test_build_wp_api_client_fixtures test goes thru each endpoint and requests it from the API, tests that it returns data, and builds up the data for the mocked QUnit tests, saving the final results to tests/qunit/fixtures/wp-api-generated.js.

Add a new grunt task restapi-jsclient which runs the phpunit side data generation and the qunit tests together.

Props jnylen0, welcher.
Fixes #39264.

@netweb
8 years ago

#26 follow-up: @jnylen0
8 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening due to a couple of issues:

  • Running a different set of PHPUnit tests (e.g. full suite vs WP_Test_REST_Schema_Initialization class) generates a different fixture file, which then shows up in svn st and similar. This isn't really desirable. Ideally this file would only be updated when the API endpoints actually change, so we need to come up with a way to make the data in the generated file always the same (IDs, usernames, post titles, etc.)
  • As @netweb noted in Slack, "the grunt restapi-jsclient task should also run qunit:compiled rather than just qunit, that way when the task is run grunt build and grunt copy:qunit have already been run."

#27 in reply to: ↑ 26 @netweb
8 years ago

Replying to jnylen0:

  • As @netweb noted in Slack, "the grunt restapi-jsclient task should also run qunit:compiled rather than just qunit, that way when the task is run grunt build and grunt copy:qunit have already been run."

The 39264.8.diff takes care of this part.

@jnylen0
8 years ago

Ensure that wp-api-generated.js fixtures remain the same across test runs

#28 @jnylen0
8 years ago

39264.9.diff should be a workable approach for the first issue:

  • Set a lot more properties on our test objects - things like title, content, and excerpt - to ensure that they won't change between test runs.
  • Remove meta keys added by another test file. This was causing several properties to show up in the meta field of posts during full test suite runs, but not during runs of just the WP_Test_REST_Schema_Initialization class.
  • For any properties that can't be easily controlled (object IDs and derivatives like link), look through the fixtures array and replace them with a set of fixed values (self::$fixture_replacements in the patch).

Also generate the fixture file with JSON_PRETTY_PRINT so that future diffs are easier to follow. I didn't include the diff of the fixture file here because it is huge.

Next I'll provide some details about how I created the self::$fixture_replacements array.

@jnylen0
8 years ago

Script to list diffs of generated fixture data

#29 follow-up: @jnylen0
8 years ago

Edit: These instructions are outdated; see comment:3:ticket:41123 instead.

Here is how to use compare-wp-api-fixtures.js to generate the self::$fixture_replacements array in 39264.9.diff.

First, clear out the self::$fixture_replacements array in your WP checkout. Then run these commands:

phpunit --filter WP_Test_REST_Schema_Initialization
cp tests/qunit/fixtures/wp-api-generated.js wp-api-generated-1class.js
phpunit --filter REST
cp tests/qunit/fixtures/wp-api-generated.js wp-api-generated-REST.js
npm install deep-diff@0.3.4
node compare-wp-api-fixtures.js

Finally, paste the results of the last command into the array.

Last edited 7 years ago by jnylen0 (previous) (diff)

#30 in reply to: ↑ 29 @adamsilverstein
8 years ago

Replying to jnylen0:

Here is how to use compare-wp-api-fixtures.js to generate the self::$fixture_replacements array in 39264.9.diff.

Excellent, thanks for your continued work on this.

I gave this a try and was able to get the same mapping array in the diff. The pretty print fixture is much better. I didn’t have a chance test to verify this fixes the issue with the fixture changing - I assume you tested that?

It is a little hard for me to grok how the normalization works - some inline docs for compare-wp-api-fixtures.js would be helpful.

#31 @jnylen0
8 years ago

Yes, this should prevent the fixtures from changing, and works well in my testing.

The js file just loads two versions of the fixtures, compares them, and prints the value from the first file for any keys that differ. I'm not sure how useful it will be going forward - I just included it here for completeness.

If you like, we can talk through any of this on Slack and see what else needs to be documented/clarified.

#32 @adamsilverstein
8 years ago

  • Keywords commit added

@jnylen0 Great, thanks for explaining and including the steps to generate the data. I'm +1 to committing this!

#33 @adamsilverstein
8 years ago

39264.10.diff

  • Add the grunt restapi-jsclientchange from .8
  • add the new generated data, in preparation for commit
Last edited 8 years ago by adamsilverstein (previous) (diff)

#34 @adamsilverstein
8 years ago

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

In 40061:

REST API: improve test fixture generation, normalizing data.

Add a data normalization pass when generating data fixtures for the REST API endpoints. Ensures that the wp-api-generated.js fixture won't change between test runs. Set more default properties and use fixed values for any properties that can't be easily controlled (object IDs and derivatives like link). Generate the fixture file with JSON_PRETTY_PRINT so that future diffs are easier to follow.

Props jnylen0, netweb.
Fixes #39264.

#35 @ocean90
8 years ago

  • Keywords needs-patch added; has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

See Slack thread, JSON_UNESCAPED_SLASHES⁠⁠⁠⁠ isn't available for PHP < 5.4.

@jnylen0
8 years ago

Remove JSON_UNESCAPED_SLASHES; make phpunit exit correctly if no config file

#36 @jnylen0
8 years ago

In 39264.11.diff:

  • Remove JSON_UNESCAPED_SLASHES which is not supported on PHP < 5.4. This wasn't very important anyway.
  • Update the results of grunt restapi-jsclient.
  • Ensure that phpunit fails with a non-zero exit code if its config file is missing. Previously, when running grunt restapi-jsclient, the next Grunt task would incorrectly proceed even if its first step (the phpunit run) failed with this error.

#37 @jnylen0
8 years ago

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

In 40065:

REST API: Fix the client test fixture generation in PHP 5.2 and 5.3.

Remove JSON_UNESCAPED_SLASHES from the wp_json_encode call - this constant is not supported in PHP < 5.4, and we don't polyfill it either.

Also make the PHPUnit test suite correctly exit with a non-zero exit code when wp-tests-config.php is not present. This was causing grunt restapi-jsclient to incorrectly proceed to its second step even when the first step failed with this error.

Props ocean90.
Fixes #39264.

#38 @jnylen0
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Still no joy:

There was 1 error:

1) WP_Test_REST_Schema_Initialization::test_build_wp_api_client_fixtures
json_encode() expects exactly 1 parameter, 2 given

/home/travis/build/nylen/wordpress-develop-svn/tests/phpunit/tests/rest-api/rest-schema-setup.php:280

I'm just going to no-op this fixture file generation on PHP < 5.4, there's no reason we will need to do that anyway.

@jnylen0
8 years ago

Skip fixture file generation on PHP < 5.4; bring back JSON_UNESCAPED_SLASHES.

#39 @jnylen0
8 years ago

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

In 40066:

REST API: Skip generating the client test fixtures in PHP 5.2 and 5.3.

Follow-up to [40065] - JSON_* constants are differently unsupported in PHP 5.2 and 5.3, which caused other, more different failures.

Also bring back JSON_UNESCAPED_SLASHES because the generated output looks nicer this way.

Fixes #39264.

#40 @jnylen0
8 years ago

  • Keywords fixed-major added; needs-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 4.7 branch.

It took us a few tries to get this right, so there are 5 different commits to port: [40058], [40061], [40065], [40066], and [40077].

edit: make it 6, including [40104]

Last edited 8 years ago by jnylen0 (previous) (diff)

#41 @jnylen0
8 years ago

In 40077:

REST API: Skip generating the client test fixtures in multisite mode.

There are a couple of changes to the generated API schemas between single-site and multisite mode - for example, the url and email settings are not present in the settings endpoint (see #39005).

To avoid unexpected changes to the wp-api-generated.js fixture file, skip generating the client test fixtures when running the test suite in multisite mode.

See #39264.

#42 follow-up: @rachelbaker
8 years ago

The test in WP_Test_REST_Schema_Initialization->test_expected_routes_in_schema() is so strict that it will fail when being run alongside any WP theme or plugin that registers custom endpoints. I forsee many Travis build failures in our community if that test method is left as is.

@adamsilverstein I get that you want to make sure there are no new routes added to core without handling in the JS client - what about just checking the routes with the /wp/v2/ namespace?

Version 0, edited 8 years ago by rachelbaker (next)

@jnylen0
8 years ago

¡Una más!

#43 in reply to: ↑ 42 ; follow-up: @jnylen0
8 years ago

Replying to rachelbaker:

The test in WP_Test_REST_Schema_Initialization->test_expected_routes_in_schema() is so strict that it will fail when being run alongside any WP theme or plugin that registers custom endpoints.

39264.13.diff should resolve this.

#44 follow-up: @dd32
8 years ago

Just noting this here - Given these are only tests changes, and all active development is happening in trunk, this wouldn't normally be something worthy of being backported to the 4.7 branch.
As the REST API is new in 4.7, and I expect there'll be diverging changes/new features in trunk/4.8 then merging this to 4.7 in order to ensure that the stable release has this proper test coverage available to it too seems like the right thing to do here. If someone else doesn't, I'll happy handle the combined merge over once we're sure there's no more changes needed :)

#45 in reply to: ↑ 43 @adamsilverstein
8 years ago

Replying to jnylen0:

39264.13.diff should resolve this.

@jnylen0 - I addressed this slightly differently in https://core.trac.wordpress.org/attachment/ticket/39561/39561.4.diff - checking for the existence of each expected route in the returned routes rather than using the strict equality check. Can you review that approach?

#46 follow-up: @jnylen0
8 years ago

@adamsilverstein I prefer my approach in https://core.trac.wordpress.org/attachment/ticket/39264/39264.13.diff for this. When a new core endpoint is added, the endpoint should be added to this list and to the client fixture file. The easiest way to ensure this is to make the test fail, which will happen with a strict equality check but not with a foreach check.

I'm also curious why we need assert && assert.equal in your patch?

#47 in reply to: ↑ 46 @adamsilverstein
8 years ago

Replying to jnylen0:

@adamsilverstein I prefer my approach in https://core.trac.wordpress.org/attachment/ticket/39264/39264.13.diff for this. When a new core endpoint is added, the endpoint should be added to this list and to the client fixture file. The easiest way to ensure this is to make the test fail, which will happen with a strict equality check but not with a foreach check.

Sounds reasonable, my tests would only fail if an endpoint were removed.

I'm also curious why we need assert && assert.equal in your patch?

I added the asset && because if you load the qunit test screen via the browser, and select a "module" such as wpapi, the nav-menus.js test file still loads, and the value for assert is not set up, leaving to a JS error:

https://cl.ly/391h3r1s1V2c/Image%202017-02-21%20at%208.08.51%20AM.png

Adding the assert check ensures this error doesn't occur. When I added the nav-menus tests, I didn’t consider that the file could load without having the tests run.

Last edited 8 years ago by adamsilverstein (previous) (diff)

#48 @jnylen0
8 years ago

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

In 40104:

REST API: Ensure that tests pass if extra endpoints are registered.

Many plugins and themes use the WP core test suite to run their unit tests, so the API tests shouldn't fail if there are extra endpoints registered in non-core namespaces.

Props rachelbaker.
Fixes #39264.

#49 in reply to: ↑ 44 @jnylen0
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to dd32:

I expect there'll be diverging changes/new features in trunk/4.8 then merging this to 4.7 in order to ensure that the stable release has this proper test coverage available to it too seems like the right thing to do here.

I should have said something about this in comment:40 but this is my thinking as well.

#50 @SergeyBiryukov
8 years ago

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

In 40116:

REST API: Add QUnit tests for wp-api.js and PHPUnit fixture generation.

Add QUnit tests: verify that wp-api loads correctly, verify that the expected base models and collections exist and can be instantiated, verify that collections contain the correct models, verify that expected helper functions are in place for each collection.

The QUnit tests rely on two fixture files: tests/qunit/fixtures/wp-api-generated.js contains the data response from each core endpoint and is generated by running the PHPUnit restapi-jsclient group. tests/qunit/fixtures/wp-api.js maps the generated data to endpoint routes, and overrides Backbone.ajax to mock the responses for the tests.

Add PHPUnit tests in tests/phpunit/tests/rest-api/rest-schema-setup.php. First, verify that the API returns the expected routes via server->get_routes(). Then, the test_build_wp_api_client_fixtures test goes thru each endpoint and requests it from the API, tests that it returns data, and builds up the data for the mocked QUnit tests, saving the final results to tests/qunit/fixtures/wp-api-generated.js.

Add a new grunt task restapi-jsclient which runs the phpunit side data generation and the qunit tests together.

Props jnylen0, welcher, adamsilverstein, netweb, ocean90, rachelbaker.
Merges [40058], [40061], [40065], [40066], [40077], and [40104] to the 4.7 branch.
Fixes #39264.

Note: See TracTickets for help on using tickets.