CODEWITHSUNDEEP
phpunit-testingphpunitassertions
Łukasz Zaroda
Łukasz Zaroda

Reputation: 767

PHPUnit: Is it a bad practice to use multiple assertions on mocked methods in one test?

I'm testing an object that is designed to test if user owns a given email. So on invocation of "tryEmail" method, it sends a message with confirmation link, to the given email address. My test looks like this:

public function testSendingWasSuccessful() {

    $confirmationObject = $this->getMock('LT\EmailConfirmation\Model\ConfirmationObjectInterface');

    $testType = 'test.type';
    $testEmail = '[email protected]';
    $testData = [];

    // EmailTester should create a new confirmation object.
    $this->manager->expects(static::once())
        ->method('create')->with($testType, $testEmail)
        ->willReturn($confirmationObject);

    // Then it should send the confirmation message.
    $this->mailer->expects(static::once())
        ->method('send')->with(static::identicalTo($confirmationObject))
        ->willReturn(true);

    // And save the confirmation object.
    $this->manager->expects(static::once())
        ->method('save')->with(static::identicalTo($confirmationObject));

    $tester = new EmailTester($this->repository, $this->manager, $this->confirmationHandler, $this->mailer);

    static::assertTrue($tester->tryEmail($testType, $testEmail, $testData));
}

Now you can see what is possibly wrong with it - it contains multiple assertions. Why I decided to use those assertions inside of the one test? Because they are dependent on each other. So, the confirmation message should be sent only if new confirmation object has been created, and confirmation object should be saved only if confirmation message was send, and at the end, the output of the "tryEmail" method, using those mocked methods is being asserted.

However, I feel like I accidentally just described the implementation of the "tryEmail" method with my assertions. But it seems to be required for full coverage of this method, and making me sure that it always work as it should. I can imagine bugs passing by if I would remove any of those assertions. For example: static::identicalTo($confirmationObject) which is basically: check if the object passed to the mailer is the same as the one created before. If I would change interface of the mailer, I would have to change also this test of the EmailTester, so it seems like I'm doing something wrong here. At the same time however - how can I check for the above assertion without introducing this coupling? Or maybe should I just leave this untested?

Am I doing it right or wrong? How could I improve on it? When to use assertions on mocks really?

Added: I just had a thought - is it not true that testing class is supposed to test the implementation (if the implementation conforms with the interface)? That would mean that describing implementation in the test is actually a good thing, because it makes sure that implementation works correctly. That would also mean that level of coupling of the implementation will be carried over to the test, and is unavoidable. Am I wrong here?

Upvotes: 4

Views: 784

Answers (1)

Schleis
Schleis

Reputation: 43760

The rule of "one assertion per test" is to keep your tests focused on one particular behavior of the code being tested. Having multiple assertions in a test is not a bad thing.

When using a mock object, I prefer to have some sort of assertions on methods being replaced. That way I ensure that the system will be using the dependencies as expected.

You testing class is to confirm behaviors of your code. The assertions that you have would be any of the checks that you would do manually to ensure that the class was behaving as you expected. Since you are expecting particular methods to be called in a specific manner, you want to have an assertion for them.

Issues that I see with the test are that you have a mock object returning a mock object. This is usually a code smell that means you are not passing the correct dependencies. You could possibly move the creation of your LT\EmailConfirmation\Model\ConfirmationObjectInterface object out of the method and pass that as a dependency of your method. Replacing the first two parameters of your method with this object.

You also don't seem to be using the third parameter at all in this test so it doesn't appear to be necessary.

Upvotes: 1

Related Questions