BrainMonkey icon indicating copy to clipboard operation
BrainMonkey copied to clipboard

Expectation on "times" required for `withArgs`

Open coreyworrell opened this issue 5 years ago • 10 comments

What am I doing wrong here?

class Example
{
  public function __construct()
  {
    add_filter('template_include', [$this, 'template']);
  }
  
  public function template(string $template): string
  {
    return 'something';
  }
}

class ExampleTest extends TestCase
{
  protected function setUp(): void
  {
    parent::setUp();
    Brain\Monkey\setUp();
  }
  
  protected function tearDown(): void
  {
    Brain\Monkey\tearDown();
    parent::tearDown();
  }
  
  public function testExample()
  {
    Brain\Monkey\Filters\expectAdded('template_include')
      ->with('Example->template()');
    new Example;
  }
}

Returns

There was 1 error:

1) ExampleTest::testExample
Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_0::add_filter_template_include([0 => object(Example), 1 => 'template'], 10, 1). Either the method was unexpected or its arguments matched no expected argument list for this method

/.../vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:92
/.../vendor/brain/monkey/src/Hook/HookExpectationExecutor.php:127
/.../vendor/brain/monkey/src/Hook/HookExpectationExecutor.php:64
/.../vendor/brain/monkey/inc/wp-hook-functions.php:35
/.../src/Example.php:24 (add_filter('template_include', [$this, 'template']))
/.../tests/ExampleTest.php:50 (new Example)

coreyworrell avatar Sep 16 '19 18:09 coreyworrell

Instead of this:

Brain\Monkey\Filters\expectAdded('template_include')
      ->with('Example->template()');

try this:

static::assertTrue( has_filter( 'template_include', 'Example->template()' ) );

(Or [ Example::class, 'template' ] etc.)

This needs to go after you instantiated the object, though.

tfrommen avatar Sep 16 '19 19:09 tfrommen

Yes that works, but shouldn't the expectation work as well? It seems to be the preferred way to test actions/filters.

coreyworrell avatar Sep 16 '19 19:09 coreyworrell

I believe Brain\Monkey\Filters\expectAdded()->with() (i.e., essentially a Mockery expectation) does not make use of CallbackStringForm, which has_filter (i.e., internally the HookStorage) does.

Is this assumption corect @gmazzap? And if so, has this been done on purpose?

tfrommen avatar Sep 16 '19 19:09 tfrommen

I saw a version from a blog article that used

expectAdded('filter')->with(function ($cb) {
  return $cb[0] instanceof Example && $cb[1] === 'template';
});

But that also gave me the same result.

coreyworrell avatar Sep 16 '19 19:09 coreyworrell

@coreyworrell, as @tfrommen said, the "string representation" of a callback only works with core functions: has_action, has_filter, did_action... It does not work when using ->with because it is a method from Mockery that I don't control.

When using Mockery methods you have to stick with what Mockery offers.

The latest snippet you posted:

expectAdded('filter')->with(function ($cb) {
  return $cb[0] instanceof Example && $cb[1] === 'template';
});

Will not work because that is telling Mockery to expect the closure as the only argument, which can never be true, because the closure is instantiated right in inside the test.

The correct way should be:

expectAdded('filter')->withArgs(function ($cb) {
  return $cb[0] instanceof Example && $cb[1] === 'template';
});

In fact, withArgs accepts either the array of arguments or a closure to verify them. See http://docs.mockery.io/en/latest/reference/expectations.html#argument-matching-with-closures

Right now, I can't test if in Brain Monkey that works as expected, if you test it please let us know.

Thanks!

gmazzap avatar Sep 20 '19 09:09 gmazzap

Thanks @gmazzap! That's what I was missing, difference between with and withArgs. I refactored my test in a way that didn't require any calls to add_filter, but this looks like the correct way if I need to do so later.

coreyworrell avatar Sep 27 '19 19:09 coreyworrell

@gmazzap Just tested this again, it does not seem to work. The test passes no matter what. Even this passes:

expectAdded('wp_dashboard_setup')->withArgs(function () {
    return false;
});

coreyworrell avatar Oct 14 '19 18:10 coreyworrell

Thanks @coreyworrell

I found the issue. When using withArgs() you have to also add an expectation on the times something should happen.

For example:

expectAdded('template_include')
	->once()   // <~ this
	->withArgs(function() { return false; });

or even:

expectAdded('template_include')
	->atLeast()->once()   // <~ this
	->withArgs(function() { return false; });

When using with to verify arguments, that is not required.

At the moment I've no time to dig into the reasons of this unconsistence, but I'll try to see if it is something that can be fixed in Brain Monkey or depends on Mockery.

If not fixable in Brain Monkey, it should at least be documented.

Thanks for reporting.

gmazzap avatar Oct 15 '19 10:10 gmazzap

Thanks for looking into this and for the fix!

coreyworrell avatar Oct 16 '19 21:10 coreyworrell

This ~might be~ is very likely related to #88

gmazzap avatar Dec 12 '22 12:12 gmazzap