Raise an error in PHPunit when calling createMock where createStub is enough

[ChinaskiJr]

Feature request

Hi !

In order to enforce and clarify the usage of test doubles for everyone, I would like to know if you think it would be beneficial to implement a rule that raises an error when a mock is created without any assertions on being called (essentially making it a stub).

This rule would only be about using the correct terminology for test doubles. It should help to better understand what is going on in our test, and what is our intention.

https://docs.phpunit.de/en/10.5/test-doubles.html

In example :

<?php
// This will raise an error : 
public function testSomething(): void 
{
    $stub = $this->createMock(Foo::class);
    $stub->method('foo')->willReturn('bar');
   
    self::assertSame('bar', $stub->foo());
}

// This won't raise an error : 
public function testSomething(): void 
{
    $stub = $this->createStub(Foo::class);
    $stub->method('foo')->willReturn('bar');
   
    self::assertSame('bar', $stub->foo());
}

// This won't raise an error : 
public function testSomething(): void 
{
    $mock = $this->createMock(Foo::class);
    $mock->expects($this->once())->method('foo')->willReturn('bar');
   
    self::assertSame('bar', $mock->foo());
}

I can try to make a PR for this, but before working on it, I prefer to ask if you think it would be a good improvement.

[ChinaskiJr]

Hello @ondrejmirtes ,

We are working on this feature with @brambaud,

We are facing an issue and we would like your advices.

We first want to specify a custom MethodTypeSpecifyingExtension, that will assert that a PHPUnit\Framework\MockObject\MockObject&Object (the return type of a Test case createMock method call) that then makes a call to an expects will return a CertainMockType.

For that we're trying to implement this :

<?php


namespace PHPStan\Type\PHPUnit;

use PhpParser\Node\Expr\MethodCall;
use PHPStan\Analyser\Scope;
use PHPStan\Analyser\SpecifiedTypes;
use PHPStan\Analyser\TypeSpecifier;
use PHPStan\Analyser\TypeSpecifierAwareExtension;
use PHPStan\Analyser\TypeSpecifierContext;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Type\MethodTypeSpecifyingExtension;
use PHPUnit\Framework\MockObject\MockObject;

class MockObjectChainedExpectsMethodTypeExtension implements MethodTypeSpecifyingExtension, TypeSpecifierAwareExtension {

	private TypeSpecifier $typeSpecifier;

	public function setTypeSpecifier(TypeSpecifier $typeSpecifier): void
	{
		$this->typeSpecifier = $typeSpecifier;
	}

    /**
     * @inheritDoc
     */
    public function getClass(): string
	{
        return MockObject::class;
    }

    public function isMethodSupported(MethodReflection $methodReflection, MethodCall $node, TypeSpecifierContext $context): bool
	{
		return $methodReflection->getName() === 'expects';
	}


    public function specifyTypes(MethodReflection $methodReflection, MethodCall $node, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes
	{
		$parent = $node->getAttribute('parent');
		return $this->typeSpecifier->create($parent, new CertainMockType('CertainMockType'), $context, $scope);
    }

}

But our custom extension is never called, to be accurate, the isMethodSupported is never called.

We found that in the TypeSpecifier class, when the Node expression is \PhpParser\Node\Expr\MethodCall, then the isMethodSupported method is only called when there is only one class referenced.

As our object is referencing two classes because our instance is an intersection (signature of PHPUnit createMock method) : PHPUnit\Framework\MockObject\MockObject&Object, our extension is never called.

Why is there this limitation ? How do you think we should do in our case ?

Thanks by advance for your work and your feedback 🙂

[ondrejmirtes]

1. @ChinaskiJr This is PHPStan bug for sure, it'd make sense to call it for all involved classes, like dynamic return type extensions are. Example here: https://github.com/phpstan/phpstan-src/blob/192fefa5f09ed10a2d5e0ccc046271e960834b9d/src/Analyser/MutatingScope.php#L5907-L5939
2. I'm not sure the proposed rule is actually a good fit for phpstan-phpunit. What's the disadvantage of creating a mock where a stub would suffice?

[brambaud]

@ChinaskiJr This is PHPStan bug for sure, it'd make sense to call it for all involved classes, like dynamic return type extensions are. Example here: https://github.com/phpstan/phpstan-src/blob/192fefa5f09ed10a2d5e0ccc046271e960834b9d/src/Analyser/MutatingScope.php#L5907-L5939

We'll work on this first then! Thanks for the answer

I'm not sure the proposed rule is actually a good fit for phpstan-phpunit. What's the disadvantage of creating a mock where a stub would suffice?

We think it is important to make the intention behind test doubles clear, as it helps in understanding tests.
Stubs and mocks serve different purposes.
A stub is used to provide predefined responses to method calls without expectation about the object behavior. By using a stub, it is clear that there will be no expectations on it.
Whereas with a mock it is specifically the expectation part which interest us. Using a mock without expectations can be confusing, as it is unclear whether the lack of expectations was intentional or not. This ambiguity may potentially hide bugs/complexities.

Furthermore, it seems to go along with how PHPUnit is expected to be used.
For instance to quote Sebastian Bergmann from this interview

When test doubles were introduced into the test frameworks of the time about 20 years ago and later became popular, the terms test stub and mock object were still interchangeable. For this reason, PHPUnit initially only offered one API, the getMock() method, for creating test doubles. This was confusing and the reason why we have worked a lot on the APIs for test doubles in recent years. When reading test code, it's important to understand what it does and why it does it. For example, if a dependency is replaced by a test double for testing purposes, it is important to know why this is happening. If the component under test is only to be isolated from a dependency, a test stub should be used. If the communication between objects is to be specified and/or verified, a mock object is required. If an API for generating mock objects is used everywhere in the test code, it is only possible to recognize what the test is about at second or third glance. The strict separation of the APIs for creating test stubs and mock objects now makes this clearer.

Maybe it could be a rule enabled only if wanted 🤔

If this is considered a rule which is too opinionated for this package, we understand and we'll see to provide it in its own package to complement this one for instance.