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 comment:1:ticket:47701 and [45646] / #47701:

• tags/5.8/tests/phpunit/tests/functions/wpGetMimeTypes.php​.

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.

[hellofromTonya]

With the focus on pushing to 5.9 Beta 1 in a week, moving this to the next cycle. But as the next cycle's milestone isn't yet available, it's Future Release. Let's move this into 6.0 once it's available. But please don't let this punt stop progress. When it's ready, it can be committed.

Trac Ticket: Core-53651

## Overview

• This pull request introduces unit tests for the wp_removable_query_args function to ensure that it behaves as expected in different scenarios. Specifically, the tests verify the following key behaviors:

• Return Type: Ensures the function returns an array.
• Non-Empty Array: Confirms that the array is not empty.
• Filter Functionality: Verifies that custom filters applied to the function modify the returned array as expected.
• Reverting to Original State: Ensures that the array reverts to its original state after the custom filter is removed.

## Added Test Cases:

• test_should_return_array_when_called: Confirms that the function returns an array.

• test_should_return_non_empty_array_when_called: Verifies that the returned array is not empty.

• test_should_modify_array_when_filter_applied: Ensures that a custom filter correctly modifies the array of query arguments.

• test_should_revert_to_original_array_when_filter_removed: Checks that the array reverts to its original state after the custom filter is removed.

Hi @SergeyBiryukov , can you please take a look into this PR. Thanks.