Proposal: Verify that quick definitions for default expectations are called at least once

[danydev]

I spent quite a bit of time using Mockery in several projects in the last few years, and one of the pain points shared across those is that developers tend to use quick definitions for stubs for cases where that expectation is not crucial for the test. Then time passes, code refactoring come, tests pass but maybe some of those stubs become useless. This means useless expectations stubs accumulate over time in our unit tests.

I think there are still a quite number of cases where quick definitions are a good practice, using them in the right way makes tests clearer with less cluttering for not-so-relevant expectations, making much easier to understand the test itself.
I'm going to make a couple of example, sorry for eventual typos, but I wrote those out of my mind without an IDE.

For example a developer may use it to mock the 'log' method of a logger that is called in a test, but for which the developer didn't really care to test this behaviour.

function test_status_is_new_after_creation() {
    $logger = \Mockery::mock(Logger::class, ['log' => null]);
    $SUT = new SUT($logger);
    $status = $SUT->getStatus($event);
    $this->assertSame(Status::NEW, $status);
}

Or maybe the developer is using it to 'connect' some mocks together, for which would be impossible for the test to pass if such connection is not in place:

function test_it_retrieves_users() {
    $user = \Mockery::mock(User::class, ['getId' => 1234]); // Assuming getId is used internally in SUT to index users by id
    $SUT = new SUT([$user]);
    $users = $SUT->getUsers();
    $this->assertSame(1234, $users[0]);
}

Or maybe a developer can use it for some methods for which the response is not relevant to the specific test he's doing, but still needs to be called and thus mocked.

// this is the code to test
function canSell(): bool
{
    if ($this->inventoryManager->isSoldOut()) {
        return false;
    }
    if ($this->date > $this->availabilityDate()) {
        return false;
    }
    return true;
}

// this is a test
function test_cannot_sell_when_date_greater_than_availability_date() {
    $inventoryManager = \Mockery::mock(InventoryManager::class, ['isSoldOut' => true]); // right now we need this to return true to reach the branch we want to test, but in the future we could move the inventory check below, making this not needed anymore
    $SUT = new SUT($inventoryManager, new DateTime('tomorrow'), new DateTime('today'));
    $canSell = $SUT->canSell();
    $this->assertSame(false, $canSell);
}

Sometimes you don't need to verify that an expectation returns a specific value, the test still makes sense because something else more relevant to what you want to test happened.

function test_it_creates_payment_when_makes_sense() {
    $total = \Mockery::mock(Total::class, ['calculate' => 10]); // Assuming 'calculate' is used to create a payment but we don't really care about verifying the specific value, it's good enough for us that 'create' has been called on payment factory.
    $paymentFactory->shouldReceive('create')->once(); // This is the real expectation we care about
    $SUT = new SUT($paymentFactory, [$user]);
    $SUT->createPaymentWhenMakesSense(new DateTime());
}

Finally I can think of situations where the developer just copy-pasted a positive test to test the opposite case, but in which different branches of code are hit where some of the 'not-so-relevant' expectations are not needed. Yes, it sounds like a silly case, but it happens sometimes, it's maybe the most common for complex tests, where developers tend to start from a template and adapt it to their use case :)

I know that we could replace all of those with a classic shouldReceive()->atLeast()->once() but still, we prefer brevity over strictness when it makes sense (i.e. when non strictly meaningful for the test).

Given such a change would make a lot of tests to fail, what I'm going to propose is an opt-in settings to make quick definitions configured with a mock using 'at least once'.
If you think it's a good idea I can work on it.

What do you guys think?
CC: @davedevelopment @robertbasic

[davedevelopment]

I don't think it's a bad idea, but it feels difficult to come up with a sensible name for the configuration value etc. I'm also less inclined to add mechanisms to Mockery than I once might have been.

In general I think I'd be more interested in Mockery surfacing stats, then reporting warnings at the end of a test run? Think johnkary/phpunit-speedtrap, but it saying things like:

Mockery Report
 1. Foobar\BazTest didn't use 12 default stub definitions
 2. Foobar\BazTest stubbed Foobar\BazDependency::create, but the method does not exist

In time we could look to add other items/best practices etc.

[danydev]

I see, stats are kind of cool, but we'll loose a bit of flexibility coupling them with php unit warnings because some people will want to fail on those kind of failures some won't. And with phpunit you can either fail on any warning or not fail at all.

We could still expose some configuration to drive failures based on stats, but if so we can say that stats are orthogonal to this feature.

About the naming I can think of something like allowMockingDefaultExpectationMethodsUnnecessarily defaulted to true.

[davedevelopment]

@robertbasic what do you think?

As said, I'm not against it, but it's not something I'd ever use. I use byDefault and the quick definitions to setup sane defaults, usually for the happy path, in my setup method. Individual tests then overrule as necessary. This way, I nearly always have unused defaults.

[danydev]

@davedevelopment I just pushed the changes to make it happen. To me it looks fair enough, a useful feature that won't impact future development, maybe we can get it merged?

CC @robertbasic

[robertbasic]

I'm sorry, this totally went past me.

First, adding atLeast->once to the test doubles will turn them from stubs to mocks, as you now have expectations on them. Having the quick definitions create one or the other based on a configuration value also seems suboptimal.

The quick definitions should only be there to quickly set up some stub methods.

I don't understand your initial statement that "useless expectations accumulate over time in our unit tests". How do expectations accumulate from quick definitions, when quick definitions don't have any expectations on them?

A bit off topic, and I should totally write down my thoughts for Mockery 2.0 somewhere in a coherent format: I don't like that people confuse all the different kinds of test doubles, and just call everything "mocks". We have dummies, fakes, stubs, mocks, and spies. Mockery creates the last 3. It creates stubs and mocks with the same mock method. What I'd like to see in Mockery 2.0 are dedicated stub and mock methods, where the stub will create you a Stub, and mock a Mock. Setting an expectation on a Stub will result in an exception, not setting an expectation on a Mock will result in an exception. I believe this would resolve a lot of confusion coming from the ambiguity of the mock method.

[danydev]

Thanks @robertbasic for replying!

Fair enough, I messed up with the terminology, sounds like I should have said: "useless stub methods accumulate over time in our unit tests", sorry for the confusion.

If we take a step back, from a userland perspective, quick definitions are a quick way to define a behaviour in a quick way (as of today a stub, a default expectation). The objective of the feature should be something like: give the user a way to know that he can remove those definitions because not really needed for the test.

Now if we say it's a legit request, I can think this kind of configuration could make sense:

if (\Mockery::getConfiguration()->getQuickDefinitionsApplicationMode() === \Mockery\Configuration::QUICK_DEFINITIONS_APPLICATION_MODE_DEFAULT_EXPECTATION) {
    $mock->shouldReceive($quickdefs)->byDefault();
} else if (\Mockery::getConfiguration()->getQuickDefinitionsApplicationMode() === \Mockery\Configuration::QUICK_DEFINITIONS_APPLICATION_MODE_MOCK_AT_LEAST_ONCE) {
    $mock->shouldReceive($quickdefs)->atLeast()->once();
}

so the semantic of the new config is to change the actual behaviour of the quick definition, what actually will be defined, stub vs mock. makes sense for you? Any feedback appreciated.

CC- @davedevelopment

[danydev]

@robertbasic I don't want to bother you too much, but would be nice if I can have your feedback.

[robertbasic]

@danydev I've left feedback on the PR

[danydev]

@robertbasic replied ;)

[danydev]

@robertbasic ping

[danydev]

@robertbasic if you are available and you think it can speed up things, I'm available to talk in a chat or anything else that is more live!

[robertbasic]

@danydev I'm sorry, I'm swamped with work and personal stuff, I don't have much time to dedicate to oss

[danydev]

I completely understand, how you advice to proceed? you want to delegate discussion to @davedevelopment or maybe you think you can handle it in a few more days...or...anything else it works for you.