Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove filter test example? #41

Closed
dingo-d opened this issue Aug 21, 2018 · 2 comments
Closed

Remove filter test example? #41

dingo-d opened this issue Aug 21, 2018 · 2 comments
Assignees
Labels

Comments

@dingo-d
Copy link

dingo-d commented Aug 21, 2018

I'm going through the documentation, and I can see examples for tests that use add_filter and add_action, but I don't see any mention of testing remove_filter/action.

So am I right in assuming this is the right way to test for remove_filter in a method?

class My_Class {
  public function set_allowed_rest_headers() : void {
    remove_filter( 'rest_pre_serve_request', 'rest_send_cors_headers' );

    add_filter( 'rest_pre_serve_request', function( $value ) {
      header( 'Access-Control-Allow-Methods: OPTIONS, GET, POST, PUT, DELETE, PATCH', false );
      ...
    }
  }
}

Test:

class Rest_Tests extends InitTestCase {
  public function setUp() {
    parent::setUp();
    $this->test_class = new My_Class();
  }

  public function test_allowed_rest_headers() {
    $this->portal_functionality->set_allowed_rest_headers();

    self::assertFalse( has_filter( 'rest_pre_serve_request', 'rest_send_cors_headers' ) );
  }
}

The InitTestCase looks like this

use PHPUnit\Framework\TestCase;
use Brain\Monkey;

class InitTestCase extends TestCase {
  /**
   * Setup method necessary for Brain Monkey to function
   */
  protected function setUp() {
    parent::setUp();
    Monkey\setUp();
  }

  /**
   * Teardown method necessary for Brain Monkey to function
   */
  protected function tearDown() {
    Monkey\tearDown();
    parent::tearDown();
  }
}

The test passes, so I guess this is ok? Also I'm testing the added filter by adding

self::assertTrue( has_filter( 'rest_pre_serve_request', 'function( $value )' ) );

and this also passes, but I don't see a green bar over that line in my code coverage report.

screen shot 2018-08-21 at 12 07 20

EDIT

I added

Filters\expectAdded( 'rest_pre_serve_request' )->once()->whenHappen( function ( $value ) {
          $value;
      });

$this->portal_functionality->set_allowed_rest_headers();

static::assertFalse( has_filter( 'rest_pre_serve_request', 'rest_send_cors_headers' ) );
static::assertTrue( has_filter( 'rest_pre_serve_request', 'function( $value )' ) );

To my test function and still my second filter isn't covered. Also, how do you test anonymous functions?

@gmazzap
Copy link
Collaborator

gmazzap commented Aug 22, 2018

Hi @dingo-d

let's go in order.

Test remove_action / remove_filter

Your example is correct. As documented, you can use has_filter like you'd do in WordPress, so if you have code that remove an hook, at the end of the test you can use has_action / has_filter to test it was actually removed.

Another possible way is to re-define remove_filter yourself and use expectations on it:

public function test_allowed_rest_headers() {
    
    Functions\expect( 'remove_filter' )
         ->once()
         ->with( 'rest_pre_serve_request', 'rest_send_cors_headers' );

     Filters\expectAdded( 'rest_pre_serve_request' )
         ->once()
         ->with( \Mockery::type( \Closure::class ) );

    (new My_Class())->set_allowed_rest_headers();
}

This is not more nor less "correct" then the way you are using, it is just different, you can do what better fits your taste.

Code coverage

That is the expected behavior. The closure that is added to the filter is not executed. The call to add_filter is executed, passing that closure as argument, but the closure itself never runs.

In other words, with this test (no matter how do you write it), you are testing that the filter is added, not that callback added to the filter is executed.

And that also what WordPress does: when you call add_filter WordPress does not execute the callback, in fact, the callback might be never executed if the matching apply_filters is executed or if matching apply_filters was executed before the add_filter call.

If you really want to execute the function you could do:

Filters\expectAdded( 'rest_pre_serve_request' )
    ->once()
    ->whenHappen( function ( $callback ) {
        $callback();
    });

And that would execute the function so your coverage turns green, but:

  • are you sure you want to execute header calls inside a test?
  • the fact the callback is excuted does not / should not change the result of the test: again, you are testing that the filter is added, and test already passed
  • the fact that the coverage is red is correct: with this test the correctness of the anonymous function is not tested. You can cheat and make coverage become all green... but that would be misleading: if you want to test the callback correctness, you have to test the callback, not the fact that the callback is added to a filter.

How do you test an anonymous function?

Actually, you really can't, not in unit testing. Yes, there might be way to execute the callbacks... but the test becomes hard to read and maintain.

This is why you should use anonymous function for things that are logic-free or maybe wrapper to other callbacks you can test separately.

E.g. if you have a code like:

add_filter( 'rest_pre_serve_request', function( $value ) {

    $sender = MyCustomHeadersSender();
    return $sender->send();
}

The 2 lines of the anonymous function will not be covered by the tests, but that's not a big issue if MyCustomHeadersSender class is tested separately.

Complex logic in anonymous function should really be avoided. When an anonymous function start growing is probably time to extract to a separate function (or class or method).

In your case, I think that usage of anonymous function is totally fine. But because of the nature of the anonymous function, which is directly interacting with HTTP layer, you should not test it in unit tests, but in a tests that executes the code in HTTP context, loading WordPress.

Unit tests do not replace integration / system / end-to-end tests... Unit tests are one of the kind of software testing, one that is surely important because enable fast development iteractions, but they are not the only kind of test, and more often than not they not suffice to ensure correctness of the software, expecially if the software interacts with side effects, complex output or different application layers like HTTP or database.

@gmazzap gmazzap self-assigned this Aug 22, 2018
@dingo-d
Copy link
Author

dingo-d commented Aug 23, 2018

First of all, thanks for the detailed answer :) I'm a bit of a newbie when it comes to testing, so I'm not all clear on some things. Your answer helped a lot.

I was suspecting I cannot test the anonymous functions but just wanted to be clear. I will add integration tests after I finish writing the unit tests because I'll need to mock a lot of things in the integration tests later on (I'm basically using WP for backend only so there are a lot of things to test).

Once I add the integration tests for the headers, these lines should become green, no?

I was even thinking of rewriting the function with the headers as a separate private function and adding it to the add_filter, but I'm not sure if this will work correctly (will have to test this, no pun intended :D ).

As for unit testing headers, I think that would have to be mocked (I might be wrong), but then the test wouldn't make much sense, as you mentioned. I'll just explain to the client that some things cannot be fully tested, but if the front end app works (which it does) it's basically a test so it's all ok :)

Thanks again for the great answer.

@dingo-d dingo-d closed this as completed Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants