Customizer Menus: Complete PHPUnit tests

[designsimply]

The Customizer Menus feature needs PHPUnit tests.

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

[ocean90]

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

[valendesigns]

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.

[obenland]

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

[valendesigns]

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

[valendesigns]

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]

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

[valendesigns]

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

[valendesigns]

Quick refresh to reflect changes made in 33089.

[welcher]

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

[welcher]

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]

Updated/cleaned up ajax unit tests

[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.

[welcher]

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]

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

[welcher]

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

[westonruter]

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.

[valendesigns]

@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]

Resolve conflicts

[westonruter]

@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.

[welcher]

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

[valendesigns]

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]

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]

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.

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/820ff44dbd8378c688a9ce7ef5beac7d028a33f7

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/820ff44...a5d6a8d

[westonruter]

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.