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

PHPUnit - Spy assertions aren't updating expectation count #785

Closed
dallincoons opened this issue Sep 11, 2017 · 26 comments
Closed

PHPUnit - Spy assertions aren't updating expectation count #785

dallincoons opened this issue Sep 11, 2017 · 26 comments
Labels
Bug An error or unexpected behavior. unreproducible

Comments

@dallincoons
Copy link

When using mocky assertions with PHPUnit 6.3:

$eventListener = \Mockery::mock(EventListenerInterface::class)
            ->shouldNotReceive('handle')
            ->getMock();

I can update the assertion count by doing something like

protected function tearDown()
    {
        if ($container = \Mockery::getContainer()) {
            $this->addToAssertionCount($container->mockery_getExpectationCount());
        }

        parent::tearDown();
    }

Which is kind of annoying but ok. As long as it gets rid of the warning about no assertions.

This doesn't work however:

$eventListener = \Mockery::spy(EventListenerInterface::class);
$eventListener->shouldHaveNotReceived('handle')

Since apparently spy assertions don't update the exceptations and thus the expectation count.

Is there a better way to go about this, other than doing something likebeStrictAboutTestsThatDoNotTestAnything="false"?

@davedevelopment
Copy link
Collaborator

There's nothing better you can do that I can think of, this is a bug that needs fixing.

@MarijnKoesen
Copy link
Contributor

If you use the MockeryPHPUnitIntegration trait in your test class you don't need to do the tearDown(), it's already done for you.

But I think you have a typo in your code, you shouldn't call shouldHaveNotReceived , but you should call shouldNotHaveReceived. That will increase the expectation count.

Basically in your code example it's like calling the method on the EventListener instead of setting up Mockery:

This is fine:

class GreenTests extends \PHPUnit\Framework\TestCase
{
    use MockeryPHPUnitIntegration;

    public function testNotReceived()
    {
        $eventListener = \Mockery::spy(EventTrackerInterface::class);
        $eventListener->shouldNotHaveReceived('handle');
    }
}

This fails with 'no test' error:

class RedTests extends \PHPUnit\Framework\TestCase
{
    use MockeryPHPUnitIntegration;

    public function testNotReceived()
    {
        $eventListener = \Mockery::spy(EventTrackerInterface::class);
        $eventListener->shouldHaveNotReceived('handle');
    }
}

So I think this is a non-issue /copy @davedevelopment @dallincoons

@alexciarlillo
Copy link

alexciarlillo commented Dec 26, 2017

Both of these result in "risky" tests for me:

public function testHandle()
    {
        $service = Mockery::spy(PlacementService::class);
        $service->handle();
        $service->shouldHaveReceived('handle');
    }

public function testHandle()
    {
        $service = Mockery::spy(PlacementService::class);
        $service->shouldNotHaveReceived('handle');
    }

I am using the integration trait. I only see this issue with Spies, not Mocks.

@Merenptah
Copy link

Same problem here. Looking into the code it seems like the directors for shouldHaveReceived and shouldNotHaveReceived are not added to the expectations via Mock::mockery_setExpectationsFor, hence they are ignored in the expectation count. Furthermore these spy methods use the VerificationDirector, this one does not have the method getExpectationCount.

@roberto-aguilar
Copy link

roberto-aguilar commented Jan 31, 2018

In sebastianbergmann/phpunit#2484 (comment), @sebastianbergmann mentioned that there is a @doesNotPerformAssertions annotation that can help in this situations.

captura de pantalla 2018-01-31 a la s 09 46 15

@Merenptah
Copy link

Well, yes, but this is not better than manually putting $this->addToAssertionCount(1), isn't it? ;) Because the test does perform assertions.

@robertbasic
Copy link
Collaborator

@Merenptah shouldHaveReceived and shouldNotHaveReceived both do increase the expectations count:

$this->_mockery_expectations_count++;

and

$this->_mockery_expectations_count++;

which then gets added to the total count:

$count = $this->_mockery_expectations_count;
foreach ($this->_mockery_expectations as $director) {
$count += $director->getExpectationCount();

Our tests show that assertion count does get incremented:

/** @test */
public function itIncrementsExpectationCountWhenShouldHaveReceivedIsUsed()
{
$spy = m::spy();
$spy->myMethod('param1', 'param2');
$spy->shouldHaveReceived('myMethod')->with('param1', 'param2');
$this->assertEquals(1, $spy->mockery_getExpectationCount());
}
/** @test */
public function itIncrementsExpectationCountWhenShouldNotHaveReceivedIsUsed()
{
$spy = m::spy();
$spy->shouldNotHaveReceived('method');
$this->assertEquals(1, $spy->mockery_getExpectationCount());
}

Both of these examples pass with 3 assertions:

<?php declare(strict_types=1);

use PHPUnit\Framework\TestCase;

require_once 'vendor/autoload.php';

class Issue785TearDownTest extends TestCase
{
    public function tearDown()
    {
        if ($container = \Mockery::getContainer()) {
            $this->addToAssertionCount($container->mockery_getExpectationCount());
        }

        parent::tearDown();
    }

    public function testSpy()
    {
        $foo = \Mockery::spy('Foo');
        $foo->shouldNotReceive('spam');

        $foo->bar();

        $foo->shouldHaveReceived('bar');
        $foo->shouldNotHaveReceived('baz');
    }
}
<?php declare(strict_types=1);

use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
use PHPUnit\Framework\TestCase;

require_once 'vendor/autoload.php';

class Issue785Test extends TestCase
{
    use MockeryPHPUnitIntegration;

    public function testSpy()
    {
        $foo = \Mockery::spy('Foo');
        $foo->shouldNotReceive('spam');

        $foo->bar();

        $foo->shouldHaveReceived('bar');
        $foo->shouldNotHaveReceived('baz');
    }
}
robert@odin ~/projects/mockery[fix/issue785*]$ ./vendor/bin/phpunit adhoc-tests/issue785Test.php 
PHPUnit 6.4.1 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.13 with Xdebug 2.5.5
Configuration: /home/robert/projects/mockery/phpunit.xml

.                                                                   1 / 1 (100%)

Time: 335 ms, Memory: 10.00MB

OK (1 test, 3 assertions)

Generating code coverage report in HTML format ... done
robert@odin ~/projects/mockery[fix/issue785*]$ ./vendor/bin/phpunit adhoc-tests/issue785TearDownTest.php 
PHPUnit 6.4.1 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.13 with Xdebug 2.5.5
Configuration: /home/robert/projects/mockery/phpunit.xml

.                                                                   1 / 1 (100%)

Time: 334 ms, Memory: 10.00MB

OK (1 test, 3 assertions)

Generating code coverage report in HTML format ... done

I honestly don't see a bug here.

I'm going to close this issue as not reproducible. If you still think this is a bug, please open a new issue providing mockery version used, php version, phpunit version, the test case where this bug occurs and your phpunit.xml file as well.

Thank you!

@robertbasic robertbasic added unreproducible Bug An error or unexpected behavior. labels Feb 3, 2018
@Merenptah
Copy link

True for Mockery 1.x. But a lot of people cannot use this, because they cannot use php 7.

@robertbasic
Copy link
Collaborator

Mockery 1 supports php 5.6

@Merenptah
Copy link

Merenptah commented Feb 3, 2018

Still on 5.5, nothing I can do against this ;) But it's ok, I can cope with this behavior.

@rafaelbeckel
Copy link

Same here.

PHP 7.1.14-1+ubuntu14.04.1

It's a Cloud9 IDE Docker container.

@robertbasic
Copy link
Collaborator

@rafaelbeckel what version of Mockery?

@rafaelbeckel
Copy link

rafaelbeckel commented Feb 25, 2018

@robertbasic

"phpunit/phpunit": "^7.0"
"mockery/mockery": "^1.0"
PHP 7.1.14-1
Ubuntu14.04.1

@robertbasic
Copy link
Collaborator

@rafaelbeckel that's interesting... Do you have any code that I can use to reproduce the issue? Could you maybe set up a test repository with the minimal code that it takes to show the bug? That would be really helpful, thank you!

@rafaelbeckel
Copy link

    public function test_Router_calls_the_right_controller_and_action()
    {
        $request = new Request( 'GET', '/foo' );
        $router = new Router([[ 'GET', '/foo', 'Bar@baz' ]]);
        
        // Router will ask the factory to build the correct Controller 
        $factory = m::mock(ControllerFactory::class);
        $factory->shouldReceive('build')
                ->with('Bar')->once()
                ->andReturn(m::spy(Controller::class));
                
        // Let's call our router
        $router->setControllerFactory($factory);
        $controller = $router->getControllerFor($request);
        
        // Router should have called the right action in the controller
        $controller->shouldHaveReceived('setAction')->with('baz')->once();
        $controller->shouldHaveReceived('setArguments')->with([])->once();
    }

Results in:

There was 1 risky test:

1) App\Http\RouterTest::test_Router_calls_the_right_controller_and_action
This test did not perform any assertions

@rafaelbeckel
Copy link

Is this related to #742 and #769?

@robertbasic
Copy link
Collaborator

@rafaelbeckel in the example you provided, when are you calling Mockery::close()?

@rafaelbeckel
Copy link

In the tearDown() method.
I also tried to include $m::close() at the end of the test itself, but with the same results.

@rcwsr
Copy link

rcwsr commented Sep 27, 2018

I think I have a similar issue upgrading to phpunit 6 and mockery 1.1.0, with tests written for phpunit 4 and mockery ~0.9

in setUp() I have this stub:

$this->serializer->shouldReceive('deserialize')
        ->with(['something'], SomeClass::class)
        ->andReturn($this->theClass)
        ->byDefault();

My test looks like this, and used to pass:

$this->serializer->shouldReceive('deserialize')
        ->once()
        ->with(['something'], SomeClass::class)
        ->andReturn($this->theClass)
        ->byDefault();

$this->repository->getById(321);

But now the tests fail with This test did not perform any assertions. I have 100s of these tests to fix! Is it that byDefault() works differently now? If I remove byDefault in the test, it passes.

I'm using the MockeryPHPUnitIntegration trait too.

@robertbasic
Copy link
Collaborator

@rcwsr Please read the documentation section on PHPUnit listeners, that should give you a good hint what to do.

@rcwsr
Copy link

rcwsr commented Sep 27, 2018

@rcwsr Please read the documentation section on PHPUnit listeners, that should give you a good hint what to do.

Thanks but that didn't really help. I am already using the TestListener, and calling Mockery::close() in tearDown directly doesn't affect the outcome.

@robertbasic
Copy link
Collaborator

Before the 1.0.0 release, Mockery provided a PHPUnit listener that would call Mockery::close() for us at the end of a test. This has changed significantly since the 1.0.0 version.

Now, Mockery provides a PHPUnit listener that makes tests fail if Mockery::close() has not been called. It can help identify tests where we’ve forgotten to include the trait or extend the MockeryTestCase.

This means that you don't call the Mockery::close() method now and you have to call it. To do that, the easiest way is to extend \Mockery\Adapter\Phpunit\MockeryTestCase or use the trait \Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration. It's written in the docs, take another read.

@davedevelopment
Copy link
Collaborator

We're probably not inspecting the defaults when adding to the assertion count?

@rcwsr
Copy link

rcwsr commented Sep 27, 2018

It's written in the docs, take another read.

@robertbasic please read my replies: I am using the MockeryPHPUnitIntegration trait, I am using the TestListener and I have also tried explicitly calling Mockery::close() in the test.

@davedevelopment
Copy link
Collaborator

@rcwsr @robertbasic see #910

@robertbasic
Copy link
Collaborator

@rcwsr sorry, somehow I've read that you're using the trait as that you're using the listener. Brains are weird.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An error or unexpected behavior. unreproducible
Projects
None yet
Development

No branches or pull requests

9 participants