10up / wp_mock

WordPress API Mocking Framework
https://wp-mock.gitbook.io
Other
671 stars 70 forks source link

Unexpected use of apply_filters for filter #225

Closed petermorlion closed 1 year ago

petermorlion commented 1 year ago

I might be using the library wrong, but essentially I want to do something like this:

WP_Mock::userFunction('apply_filters')
    ->with('my_filter', 'arg1', 100, 6, 123)
    ->andReturnUsing(function($filter, $arg1, $arg2, $arg3, $result) {
        return $result;
    });

However, I get an error:

Unexpected use of apply_filters for filter my_filter

I know there is a onFilter function, but that doesn't seem to return a Mockery Expectation and I can't use andReturnUsing on it.

But even if I use it, it doesn't work. I use it like this:

WP_Mock::onFilter('my_filter')
    ->with('arg1', 100, 6, 123)
    ->reply(123);

Ideally, I'd even want to omit the with line and have something like this:

WP_Mock::onFilter('my_filter')
    ->replyUsing(function(arg1, $arg2, $arg3, $result) {
        return $result;
    });
unfulvio-godaddy commented 1 year ago

hey @petermorlion do you have a snippet of the code under test?

If you want to check that an apply_filters() is invoked, ie that a filter is registered, you can use the WP_Mock::expectFilterAdded()method: https://wp-mock.gitbook.io/documentation/usage/mocking-wp-action-and-filter-hooks#ensuring-actions-and-filters-are-registered

If you want to test that a filter is applied via add_filter() you can use onFilter() as explained here https://wp-mock.gitbook.io/documentation/usage/mocking-wp-action-and-filter-hooks#asserting-that-actions-and-filters-are-applied

Please let me know if something is not clear or if I missed anything.

petermorlion commented 1 year ago

Hi @unfulvio-godaddy, thanks for the swift reply.

Basically, this is the piece of code I want to test (anonimized):

final class Calc
{
    static function get_result($arg1, $total, $percentage)
    {
        $result = 0;
        $result = $total * $percentage; // or some other calculation

        $result = apply_filter('my_plugin_calc', $arg1, $total, $percentage, $result);

        return $result;
    }
}

I have a test like this:

WP_Mock::onFilter('my_plugin_calc')
    ->with('something', 100, 0.5, 50)
    ->reply(50);

$this->assertEquals(5.66, Vat::get_VAT('PAKKET', 100, 6));

Which produces the Unexpected use of apply_filters for filter my_plugin_calc error. And I would prefer not to have to write the expected arguments of the filter call in my tests, because that adds knowledge about the implementation to my tests. I just want the filter call to do nothing but return the $result as it was passed in.

Is this possible?

unfulvio-godaddy commented 1 year ago

I see some issues with your example, better check that the code is correct:

  1. The WordPress function to register a filter hook is apply_filters (plural) and not apply_filter (singular)
  2. Your test method example has Vat::get_VAT() as the function call under test but you have defined above Calc::get_result() - I gather this might be just an example but just make sure you are invoking the correct function in the test

I did test a similar scenario (also with final classes and static methods) and it seemed to work for me.

Note that you shouldn't be using WP_Mock::userFunction('apply_filters') in your tests when mocking filters, that would be taken care of by functions like WP_Mock:expectFilterAdded() or WP_Mock::onFilter().

Finally, have you bootstrapped WP_Mock with WP_Mock::bootstap() in your bootstrap.php file used by PhpUnit?

petermorlion commented 1 year ago

Ok, this has pointed me to a bug in my code, so WP_Mock is working fine as you say. Thanks. The issue was that I was doing number_format instead of round in my calculation (PHP novice here).

Though I still wonder if I could omit the specific values of the arguments, because that adds knowledge of the implementation in my test, and I would want it to be a black-box test. If I replace an argument in the with call with \Mockery::any() may test fails again.

unfulvio-godaddy commented 1 year ago

Thanks for confirming that it's working for you.

Though I still wonder if I could omit the specific values of the arguments, because that adds knowledge of the implementation in my test, and I would want it to be a black-box test.

It depends what kind of test do you want to perform. By default, the responsibility that apply_filters() will, at minimum, pass through the 1st argument passed to it is delegated to WordPress own tests, so you could ignore performing assertions or setting expectations with regards to apply_filters(). If you run apply_filters(), WP_Mock should be able to let it run and return the first argument, while you just take care of test the rest of your logic (this is the default WP behavior when apply_filters() is invoked, unless 3rd party code uses add_filter() to interact with it). Then, you can still use onFilterAdded to simulate the effects of a third party applying a filter; in that case you need to pass the arguments that this filter will apply on your registered filter via apply_filters(). More or less like setting expectations on Mockery.

petermorlion commented 1 year ago

In my test, there is no 3d party that is hooking into the filte. Also, I'm not using WordPress tests, but PHPUnit. What I'm trying to do is have the apply_filters call just return the last argument, regardless of the value of the others. I need to get the raw result of my calculation, without filters applied. How could I do this with the current WP_Mock? Is it possible?

The code from my first post would be useful, IMO:

WP_Mock::onFilter('my_filter')
    ->replyUsing(function(arg1, $arg2, $arg3, $result) {
        return $result;
    });

Meaning that when a my_filter filter is applied, WP_Mock runs the function we passed in, which just returns the $result that the core calculation calculated.

unfulvio-godaddy commented 1 year ago

By WordPress tests I meant the responsibility of testing apply_filters lies inside the WordPress project and probably you shouldn't be concerned in testing its functionality other than if you are trying to add a filter and see how the behavior of your code should change when a 3rd party interacts with it (say the variable handled by apply_filters() is of a different type, for example) -- which is perhaps what you're trying to do as per your example in your last comment.

Unfortunately it doesn't seem possible to use WP_Mock::onFilter() as you describe. If you want to test the logic of your callback method or function, that needs to be done in a separate test.

For the purposes of testing the effects of a filter intervening and changing the variable passed to apply_filters() all you need is to change this variable passed to reply() and write the rest of the test accordingly, via mocks or expectations, etc. If you need to be concerned how that value is determined, I suggest to test that separately.

For example if you have a code that adds add_filter('myFilter', [$this, 'myMethod']); you can write a PHPUnit test for myMethod.

unfulvio-godaddy commented 1 year ago

hey @petermorlion please let me know if you have solved your problem and if this can be closed, thanks!

petermorlion commented 1 year ago

Hi @unfulvio-godaddy , thanks for bearing with me. The thing is, I want to test the logic of my function, and I don't care about the filter. Let's get back to the beginning, to clarify. Say I have this code:

final class Calc
{
    static function get_sum($arg1, $arg2)
    {
        $result = 0;
        $result = $arg1 + $arg2;

        return $result;
    }
}

You'd test it like this:

final class CalcTest extends TestCase
{
    public function test_get_sum(): void
    {
        $this->assertEquals(6, Calc::get_sum(4, 2));
    }
}

Simple, this works. But what if I change my code and add a filter:

final class Calc
{
    static function get_sum($arg1, $arg2)
    {
        $result = 0;
        $result = $arg1 + $arg2;

        $result = apply_filters('my_plugin_sum', $arg1, $arg2, $result);

        return $result;
    }
}

How do I properly test this now? If I don't do anything, I get the Exception that apply_filters was called. I could add an onFilter call, but that would mean I'm adding knowledge of the apply_filter call to my test:

final class CalcTest extends TestCase
{
    public function test_get_sum(): void
    {
        WP_Mock::onFilter('my_plugin_sum')
            ->with(4, 2, 6)
            ->reply(6);

        $this->assertEquals(6, Calc::get_sum(4, 2));
    }
}

This may be me being pedantic, but I don't like adding this knowledge to my test. The test should just assert that entering 4 and 2 as input, and if there are no filters present, it should result in 6.

So just to be sure, this isn't possible, and I have to call the onFilter->with with the expected arguments? If so, I think having a replyUsing would be a nice addition as I've seen it possible in most other programming languages I've used. But if you don't plan on adding this, I respect that!

unfulvio-godaddy commented 1 year ago

hey @petermorlion I think you could use a @dataProvider this is a PHPUnit feature, not a WP_Mock feature. See https://docs.phpunit.de/en/9.6/writing-tests-for-phpunit.html#data-providers


    /**
     * @covers \MyApp\MyNamespace\Calc::get_Sum()
     * @dataProvider providerGetSum
     * 
     * @param int $arg1
     * @param int $arg2
     * @param int $expectedResult
     * @return void
     */
    public function testGetSum(int $arg1, int $arg2, int $expectedResult): void
    {
        $this->assertSame($expectedResult, Calc::get_sum($arg1, $arg2));
    }

    public function providerGetSum() : Generator
    {
        yield '1 + 1 = 2' => [
            'arg1' => 1,
            'arg2' => 1,
            'expectedResult' => 2,
        ];

        // other cases according to your math calc
    }

In this test you are asserting that provided $arg1 = 1 and $arg2 = 2, the get_sum function should return 2 according to the method's logic (for example sake) - as you can see you are not informing the test of any logic being performed, you are only interested in the output of that method matches the prediction based on your inputs

The filter is completely irrelevant because by default it only passes through the 1st argument assigned to it $arg1. As a matter of fact, I think you are not using apply_filters() correctly - the way you h ave written it, it will assign $arg1 to $result. See https://developer.wordpress.org/plugins/hooks/

You could add an assertion in your test to test that there's an apply_filters call therein by adding a conditional $hasFilter and execute WP_Mock:onFilter() conditionally, and change the predicted normal outcome. That way you can assert that, if a filter is present, then the function returns something unexpected. For example:

    public function testGetSum(int $arg1, int $arg2, $onFilter, int $expectedResult): void
    {
        if (null !== $onFilter) {
            WP_Mock::onFilter('my_plugin_sum')
                ->with($arg1, $arg2)
                ->reply($onFilter);
        }

        $this->assertSame($expectedResult, Calc::get_sum($arg1, $arg2));
    }

If you have a method in your code that adds a callback for your filter, you can add a test method that tests the logic of that callback as you would do with any other method.

petermorlion commented 1 year ago

Ok, I'm making progress. I changed my code to put the result of the function as the first argument of the filter, as per the convention. Thanks for pointing that out.

I still need to call WP_Mock::onFilter which is too bad, but I have fixed my issue. You can close it if you want.

unfulvio-godaddy commented 1 year ago

hey @petermorlion thanks for letting me know and glad you were able to iron out your tests! We are looking forward to make improvements in WP_Mock, so please keep an eye on WP_Mock updates and new releases in the future. Thanks