#39264 closed enhancement (fixed)
Add WP-API JS Client unit tests to core
Reported by: | adamsilverstein | Owned by: | 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)
Change History (64)
#2
@
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
@
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
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
8 years ago
#8
@
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
@
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
@
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
@
8 years ago
- 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
@
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
@
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.
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
@
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.js
→wp-api-generated.js
wp-api-2.js
→wp-api.js
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
8 years ago
#24
@
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.
#26
follow-up:
↓ 27
@
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 insvn 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 runqunit:compiled
rather than justqunit
, that way when the task is rungrunt build
andgrunt copy:qunit
have already been run."
#27
in reply to:
↑ 26
@
8 years ago
Replying to jnylen0:
- As @netweb noted in Slack, "the
grunt restapi-jsclient
task should also runqunit:compiled
rather than justqunit
, that way when the task is rungrunt build
andgrunt copy:qunit
have already been run."
The 39264.8.diff takes care of this part.
#28
@
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 theWP_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.
#29
follow-up:
↓ 30
@
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.
#30
in reply to:
↑ 29
@
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
@
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
@
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
@
8 years ago
- Add the
grunt restapi-jsclient
change from .8 - add the new generated data, in preparation for commit
#35
@
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.
#36
@
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 runninggrunt restapi-jsclient
, the next Grunt task would incorrectly proceed even if its first step (thephpunit
run) failed with this error.
#38
@
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.
#40
@
8 years ago
- Keywords fixed-major added; needs-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
#42
follow-up:
↓ 43
@
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?
#43
in reply to:
↑ 42
;
follow-up:
↓ 45
@
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:
↓ 49
@
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
@
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:
↓ 47
@
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
@
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:
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.
#49
in reply to:
↑ 44
@
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.
I chatted in person with @mboynes and he said he may have some ideas for this, so tagging him in.