unit test for wp_removable_query_args

[pbearne]

Added missing unit test

Trac ticket: https://core.trac.wordpress.org/ticket/53651#ticket

[johnbillion]

Thanks for the PR @pbearne but I question how useful these tests are. What's being tested exactly?

• The test_wp_removable_query_args() test only confirms that the fixed set of values in the function matches the same fixed set of values in the test, and doesn't add assurance of anything.
• The test_wp_removable_query_args_filter() test is only testing that a filter works, which is unrelated to the function being tested.

I'm all for adding more tests, but as-is these don't provide any value.

[pbearne]

@johnbillion

Part of the drive is to have complete coverage for all functions.

I agree it seems silly to test the that all the same values as this increases the maintenance as to add another value we would need to do it in two places.
Would just checking that we get an array back be sufficient or should check the shape of the array as well?

@jrf @hellofromtonya
I have been struggling with how to make sure that the expected filter(actions) are in the functions.

Filters and Actions are a key part of how WP works and a lot will break if they get changed.

If the output of a function has a filter should we not check that is applied? A set of tests that doesn't check will still pass if the filter got renamed/deleted or an early return was added.

Throught?

Paul

[SergeyBiryukov]

This reminds me of the wp_get_mime_types() test​, see comment:1:ticket:47701 and [45646] / #47701.

I think something similar would be enough here:

• Make sure it returns an array.
• Make sure it's not empty.
• Make sure the filter works as expected.
• Make sure it reverts to the original array after removing the filter.

While the latter two steps might technically be out of scope here, I agree that if the function result is supposed to run through a filter, testing for that specifically seems like a good idea.

[jrf]

I've just had a look at the PR and at the function.

The function does not actually contain any logic, so there isn't much to test in reality.

I agree with @johnbillion that checking the exact content of the array in the first test does not add value.
Just checking that the return value of the function is an array and that that array is not empty should be sufficient.
The actual values within the array should not be tested like this.

I also agree with @SergeyBiryukov that testing that the filter gets applied is a good idea, though I don't agree that the filter needs to be removed and the output checked again after. That is something which should be tested in the Hooks logic, not here.

I also don't think we should care much about what the filter returns as that is defined within the test. We just need to check that the returned filtered value complies with what the test filter does. That confirms that the filter has been applied.
Testing whether the function doing the filtering is returning the expected type and is not empty and such, doesn't add any value as we don't control outside filters, so testing this doesn't actually yield any extra code security.

I would suggest simplifying the filter test to this (also note the test function name change):

<?php
public function test_wp_removable_query_args_applies_filter() {
        add_filter( 'removable_query_args', static function( $args ) { return array(); } );

        $this->assertSame( array(), wp_removable_query_args() );
}

All filter adjustments get automatically reverted via the abstract TestCase tear_down() method anyway, so we don't have to worry about using a closure which cannot be removed via remove_filter() in the tests.