Since v1.5.1 I'm getting "This test did not perform any assertions"

[simPod]

I compared 1.5.1 to 1.5.0 and it seems the only function change is this #1180

I'm getting "This test did not perform any assertions" for tests that contain ->shouldReceive calls.

It was counted towards assertions in all previouis Mockery versions.

[simPod]

And my main issue is that from the changelog I'm unable to find out what should I do.

[andrewbroberg]

Noticing the same issue here, could it be this change that has caused it potentially?

72cf17b

[andrewbroberg]

Found the reason why these are failing, it looks like it may be by design.

Mockery::mock(Foo::class)->shouldReceive('bar');

The above will fail as we have not provided the number of calls we expect to it, we will need to update it to something like below adding once() chained method call

Mockery::mock(Foo::class)->shouldReceive('bar')->once();

The added test in the commit show this expected behaviour 72cf17b#diff-e290c245179d001a519df9ee5cecf659df2e2ff8506829ef10dcc6900559fe56

[simPod]

@andrewbroberg that's nice actually. I guess it's a fix then?

[ghostwriter]

It was counted towards assertions in all previouis Mockery versions

this was a minor bug.

And my main issue is that from the changelog I'm unable to find out what should I do.

we'll update the changelog to reflect the changes.

I guess it's a fix then?

correct, ->shouldReceive(…) would result in zero assertions because it's only stubbing the method call.

[GrahamCampbell]

I think that is debatable. That should probably be regarded as a breaking change, and delayed till 2.x, and reverted in 1.x.

[davedevelopment]

Definitely debatable, but nobody turned up to the debate and then I forgot...

[ghostwriter]

Regarding BC Break,

could/should we internally set the default expectation for method stubs ->shouldReceive(…) to ->once() and allow it to be overwritten?

$mock = Mockery::mock(); 
$mock->shouldReceive('foo'); // Same as: $mock->shouldReceive('foo')->once();
// can be overwritten
// $mock->shouldReceive('foo')->atLeast()->once();
// $mock->shouldReceive('foo')->never();

$this->assertSame(1, Mockery::getContainer()->mockery_getExpectationCount());

• ->shouldReceive(…) would result in one assertion, addressing the issue that feat: only count assertions on expectations which can fail a test #1180 fixed.

$mock->shouldReceive("test1");
$mock->shouldReceive("test2")->atLeast()->once();
$mock->shouldReceive("test3")->once();
$mock->shouldReceive('foo')->never();

$mock->test1();
$mock->test2();
$mock->test3();

• would record four assertions.
• if the stubbed method wasn't called, it would fail verification, unless it's never() expected to be called.

Thoughts?

[andrewbroberg]

IMO this is a good change, we should be calling once or similar otherwise we're not actually asserting on anything

[davedevelopment]

Regarding BC Break,

could/should we internally set the default expectation for method stubs ->shouldReceive(…) to ->once() and allow it to be overwritten?
...
Thoughts?

If what we've already done is considered a BC break, I would also consider that a BC break.

Regardless, this is essentially one of the driving forces for what I did for the (not so) new allows and expects syntax. allows for stubs, which is actually just a proxy to the old shouldReceive and expects for expectations, which is a proxy to shouldReceive()->once().

    public function expects($something = null)              
    {                                                       
        if (is_string($something)) {                        
            return $this->shouldReceive($something)->once();
        }                                                   
                                                            
        return new ExpectsHigherOrderMessage($this);        
    }                                                       

[zlodes]

I thought that shouldReceive is deprecated 🤔
It should be replaced with expects (means shouldReceive + once) and allows (just shouldReceive).

Because should word isn't fine there.

[mfn]

I've previously commented at #1180 (comment) and only afterwards found this issue now.

Turns out the change is affecting us in a bigger way than I initially was able to see.

From a testsuite of 20k+ tests, we have ~500+ being reported as "Risky".

What added more to the confusion in our case is how different environments treated this.

• phpunit (9.5.25), with default beStrictAboutTestsThatDoNotTestAnything=true, will report them in test runs but will still return exit code 0 => that's why I initially didn't "see" the 500+ because the GHA workflow runs were green
• however we've split teststuites in phpunit and run some through paratest. paratest however will return a non-0 exit code when encountering risky tests with above settings => this is how I initially only found 1 test with a dataprovider having "an issue"
  Apparently the paratest behaviour is due to the junit log format, if that is still the most accurate information on that.
• also in phpstorm, which uses phpunits --teamcity flag, those risky tests are marked as testFailed and thus they appear as failed, when in phpunit by default they wouldn't.
  Interestingly, phpstorm could check phpunits exit code because even with --teamcity it's 0 but it honors only the testFailed status there.
  But we didn't find this out until later, because the 20k+ tests are mostly integration/feature tests and it's usually not feasible to run them locally.

---

However we also agree those tests are broken and need to be fixed, so we're not advocating for any change here (1.5.1 is released, aka "damage is done" anyway) and are simply going ahead and add a lot of expectNotToPerformAssertions() calls where previously none were 🤞🏼

[jrfnl]

Just wanted to point out that this doesn't only affect userland tests, but also the tests of Mockery itself.

I've re-run the tests for Mockery itself on the commit before PR #1180 was merged (as GH Actions had already thrown away the logs): this shows 5 tests which are marked as "risky" by PHPUnit (prior to the change).

When we compare this to a (re-run) build of the commit merging PR #1180, it shows between 39 and 44 tested marked as "risky" by PHPUnit (exact number depends on the PHP version used).

[mfn]

phpunit (9.5.25), with default beStrictAboutTestsThatDoNotTestAnything=true, will report them in test runs but will still return exit code 0 => that's why I initially didn't "see" the 500+ because the GHA workflow runs were green

FTR, no idea how I came up with this strange setting but simply adding failOnRisky="true" made them a hard stop.

We also found a couple of bugs in our test suite thanks to this change 🙏🏼

[downsider]

While not debating that this was a bug that needed fixing, the 1.5.1 patch change has caused our test suite to throw several new failure cases. That's a breaking change in no uncertain terms.

Semver would suggest that this change be delayed until the next major release. To leave it as-is has a negative impact on downstream users; this could be preventing someone from deploying an urgent bugfix to their application. Regardless of whether a test suite is technically flawed or not, we shouldn't be forcing people to update their tests just because they installed a new patch version of mockery.

For the record, this kind of change is why I prefer using Ferver over Semver. Breaking changes should not be something to be afraid of, you just have to ensure that people don't get caught out by them. In Ferver, this would be a new minor version; if people wanted to update to the latest version, they would need to consciously change composer.json and can handle the consequences of that appropriately.

we'll update the changelog to reflect the changes.

Seeing as this is outstanding, please see #1192

[BenMorel]

IMO this is a good change, we should be calling once or similar otherwise we're not actually asserting on anything

The issue is, if you have other assertions in your test, then PHPUnit will happily mark the test as successful, even though the expectation failed:

interface I {
    public function test(): void;
}

class MyTest extends TestCase
{
    public function test(): void
    {
        $i = Mockery::mock(I::class);
        $i->shouldReceive('test');

        self::assertTrue(true);

        Mockery::close(); // will not complain, and PHPUnit will mark the test as successful!
    }
}

Calling shouldReceive() alone is akin to doing nothing, which can hide potential bugs as you can see here.

I can see a couple ways to solve this:

• as suggested by @ghostwriter above:

  set the default expectation for method stubs ->shouldReceive(…) to ->once() and allow it to be overwritten

• or, store a flag in Container to mark shouldReceive() as unconfigured until once()/never()/... has been called, and throw in Mockery::close() if this flag is set

Also, if shouldReceive() is deprecated in favour of expects(), it would be nice to document it as @deprecated, so that we can at least run static analysis on our tests to detect potential issues.

Thoughts?

[Danny-Smart]

set the default expectation for method stubs ->shouldReceive(…) to ->once() and allow it to be overwritten

We often use a blank shouldReceive() to prevent unexpected invocation errors where it is not possible or desirable to use shouldIgnoreMissing(). This happens frequently where loggers are used, or for other cases where a void return method is called that has nothing to do with the behaviour under test.

If we did go down this route, I think we would also need to add an ->any() method, to allow the previous behaviour to be preserved

$mock = Mockery::mock(Example::class);
// Currently, the two following expectations would be identical
// If we assign `->once()` by default, the latter allows for method calls we don't need to worry about in this test
$mock->shouldReceive('test');
$mock->shouldReceive('test')->any();