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

Expectation on "times" required for withArgs #52

Open
coreyworrell opened this issue Sep 16, 2019 · 10 comments
Open

Expectation on "times" required for withArgs #52

coreyworrell opened this issue Sep 16, 2019 · 10 comments
Labels

Comments

@coreyworrell
Copy link

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)
@tfrommen
Copy link
Contributor

tfrommen commented Sep 16, 2019

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.

@coreyworrell
Copy link
Author

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

@tfrommen
Copy link
Contributor

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?

@coreyworrell
Copy link
Author

coreyworrell commented Sep 16, 2019

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.

@gmazzap
Copy link
Collaborator

gmazzap commented Sep 20, 2019

@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!

@coreyworrell
Copy link
Author

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
Copy link
Author

coreyworrell commented Oct 14, 2019

@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 coreyworrell reopened this Oct 14, 2019
@gmazzap
Copy link
Collaborator

gmazzap commented Oct 15, 2019

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 gmazzap added the bug label Oct 15, 2019
@gmazzap gmazzap changed the title Filter\expectAdded no matching handler found Expectation on "times" required for withArgs Oct 15, 2019
@coreyworrell
Copy link
Author

Thanks for looking into this and for the fix!

@gmazzap
Copy link
Collaborator

gmazzap commented Dec 12, 2022

This might be is very likely related to #88

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

3 participants