WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#32687 closed task (blessed) (fixed)

Customizer Menus: Complete PHPUnit tests

Reported by: designsimply Owned by: valendesigns
Milestone: 4.3 Priority: high
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

The Customizer Menus feature needs PHPUnit tests.

Moved from https://github.com/voldemortensen/menu-customizer/issues/68

Attachments (11)

32687.diff (9.8 KB) - added by valendesigns 6 years ago.
32687.2.diff (11.7 KB) - added by valendesigns 6 years ago.
Move param sanitization to the correct method and clean up unit tests.
32687.3.diff (12.0 KB) - added by valendesigns 6 years ago.
32687-ajax-tests.diff (7.2 KB) - added by welcher 6 years ago.
Adding nav-menus-ajax.php and creating initial tests for ajax_load_available_items
32687-ajax-tests.2.diff (8.3 KB) - added by welcher 6 years ago.
Updated/cleaned up ajax unit tests
32687-ajax-tests.3.diff (13.9 KB) - added by welcher 6 years ago.
Completed Tests for the 2 ajax methods in nav-menus.php
32687.4.diff (11.5 KB) - added by westonruter 6 years ago.
Resolve conflicts
32687.5.diff (24.3 KB) - added by valendesigns 6 years ago.
32687.6.diff (24.1 KB) - added by valendesigns 6 years ago.
32687.7.diff (24.8 KB) - added by westonruter 6 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/820ff44dbd8378c688a9ce7ef5beac7d028a33f7
32687.8.diff (24.8 KB) - added by westonruter 6 years ago.
Additional changes: https://github.com/xwp/wordpress-develop/compare/820ff44...a5d6a8d

Download all attachments as: .zip

Change History (32)

#1 @ocean90
6 years ago

  • Summary changed from Customizer Menus: add PHPUnit tests to Customizer Menus: Complete PHPUnit tests

There are actually three incomplete tests: test_render_menu, test_load_available_items_ajax and test_search_available_items_ajax.

#2 @valendesigns
6 years ago

There are likely more tests than the three I set as incomplete that we could add. Though, at the point I stopped adding tests we had 75% coverage so those other three incomplete tests would likely get us close to 85% since those methods have a decent amount of testable lines of code in them that would contribute to the overall coverage percentage. Some of the remaining methods are just not testable or do not need tests. I'll create a patch for us to iterate on, and add more incomplete tests to the ones I think we should cover in the next day or two, so we have a clear goal.

#3 @obenland
6 years ago

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

@valendesigns, could you revisit this when you get a chance?

#4 @valendesigns
6 years ago

Yeah no problem, I can look at this probably later today or tomorrow.

@valendesigns
6 years ago

#5 @valendesigns
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Version set to trunk

In 32687.diff I moved the logic that was found in ajax_load_available_items into a separate method named load_available_items_query, which is much easier to unit test, and added five tests that cover the expected return values of this new method. Plus did a little variable and doc clean up in search_available_items_query to make things consistent.

Additionally, I removed the incomplete test_render_menu, because WP_Customize_Nav_Menus::render_menu is just untestable.

These changes should theoretically bring us to 100% coverage of WP_Customize_Nav_Menus. Well, that is for the methods we can actually test.

@valendesigns
6 years ago

Move param sanitization to the correct method and clean up unit tests.

#6 @valendesigns
6 years ago

The latest patch will need to be refreshed once #32858 is committed.

@valendesigns
6 years ago

#7 @valendesigns
6 years ago

Quick refresh to reflect changes made in 33089.

#8 @designsimply
6 years ago

  • Priority changed from normal to high

@welcher
6 years ago

Adding nav-menus-ajax.php and creating initial tests for ajax_load_available_items

#9 @welcher
6 years ago

First pass for WP_Customize_Nav_Menus::ajax_load_available_items() tests.

Any feedback greatly appreciated. If this approach is acceptable, I will continue with WP_Customize_Nav_Menus::ajax_search_available_items()

Thanks!

@welcher
6 years ago

Updated/cleaned up ajax unit tests

#10 follow-up: @westonruter
6 years ago

@welcher Looks good to me! I haven't dove into the unit tests for WP Ajax in other parts of Core, but as long as you're following the existing cowpaths, then we should be great. One thing: it looks like the tests are missing an ajax test group. I believe that this has to be used instead of customize due to the special behavior of wp_send_json() when DOING_AJAX.

#11 in reply to: ↑ 10 @welcher
6 years ago

Replying to westonruter:

@welcher Looks good to me! I haven't dove into the unit tests for WP Ajax in other parts of Core, but as long as you're following the existing cowpaths, then we should be great. One thing: it looks like the tests are missing an ajax test group. I believe that this has to be used instead of customize due to the special behavior of wp_send_json() when DOING_AJAX.

@westonruter Thanks for the feedback! I'm working through some local setup issue with running the ajax tests but am hoping to get the rest of these test uploaded soon.

@welcher
6 years ago

Completed Tests for the 2 ajax methods in nav-menus.php

#12 @welcher
6 years ago

Updated patch that will now run in the ajax group. Both ajax methods are now being tested. Any feedback welcome!

This ticket was mentioned in Slack in #core-customize by valendesigns. View the logs.


6 years ago

#14 @westonruter
6 years ago

  • Keywords needs-refresh added

The attachment 32687.3.diff needs a refresh, and it needs to be merged with 32687-ajax-tests.3.diff in a unified patch. There are conflicts with the former patch because of trunk changes to wp_send_json_error() sending back a code as opposed to a message, whereas the patch is changing to return a WP_Error object. This may not be needed with the work done in the latter patch.

#15 @valendesigns
6 years ago

@westonruter It's a shame 32687.3.diff wasn't merged earlier. Its focus was on removing the logic from the AJAX like how search_available_items_query is handling the logic for ajax_search_available_items to make it more testable. You could probably commit the AJAX patch today if you think it's ready, and not wait for a refresh. The two patches are not really dependent on each other, and I'm not able to work on a new patch until tomorrow.

@westonruter
6 years ago

Resolve conflicts

#16 @westonruter
6 years ago

  • Keywords needs-refresh removed

@valendesigns: In 32687.4.diff I resolved the conflicts in your last patch with what is in trunk. I think you're right that we should try to keep the ajax group tests as small as possible because these tests aren't run by default, and they take longer to execute.

#17 @welcher
6 years ago

I'm afk for the weekend but can look at anything needed for the Ajax tests on Monday in needed.

#18 @valendesigns
6 years ago

Thanks for refreshing the patch @westonruter! I'll test 32687.4.diff this morning, also the AJAX one, and create a single merged patch for commit. Thank you @welcher for your work on the AJAX methods. After these two are combined & committed I feel we can close the ticket and put more focus on the JS tests. That's really where we're lacking, I believe the PHP is sufficiently tested.

@valendesigns
6 years ago

#19 @valendesigns
6 years ago

  • Keywords commit added; needs-testing removed

I've combined the two patches into 32687.5.diff and renamed & moved the AJAX file plus a few coding standards corrections. Both set of tests cover our AJAX requests very well and we should now be solid on PHPUnit tests since these were the only ones still left to do.

@valendesigns
6 years ago

#20 @valendesigns
6 years ago

I'm thinking that we should make a small adjustment to load_available_items_query before it's committed, so the hooks API is easier to create. Basically what I did in 32687.6.diff was remove the assertion & WP_Error that forced $obj_type to be post_type or taxonomy, which allows us to add alternative custom object types in a separate patch in #32708 which I'm refreshing now. This new hook would be in an else statement at the end of the function and filter the available $items for that custom object type. This way we're not held back by an arbitrary requirement that the object type be post_type or taxonomy which makes zero sense for extensibility purposes.

#21 @westonruter
6 years ago

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

In 33322:

Customizer: Finish unit tests for nav menus.

Removes object_type restriction to allow for future extensibility. Refactors some methods to improve testability. Includes new tests for Ajax requests.

Fixes #32687.
Props valendesigns, welcher, westonruter.

Note: See TracTickets for help on using tickets.