4 Fix type
source | link

This readyreads much nicer.

This ready much nicer.

This reads much nicer.

3 changed to use the same property throughout the example
source | link
$this->blockFactoryMock>modelBlockFactoryMock = $this->getMock(\Magento\Cms\Model\BlockFactory::class, ['create'], [], '', false);
$this->blockFactoryMock = $this->getMock(\Magento\Cms\Model\BlockFactory::class, ['create'], [], '', false);
$this->modelBlockFactoryMock = $this->getMock(\Magento\Cms\Model\BlockFactory::class, ['create'], [], '', false);
2 added 1090 characters in body
source | link

The $objectManagerMock property now is obsolete and all references to it can (actually, shouldshould) be removed from the test class.

Next, I question the use of \Magento\Framework\TestFramework\Unit\Helper\ObjectManager.
In my opinion (and that is only me) it is better to instantiate the class under test manually using new. The only thing the helper does at this time is it creates a \Magento\Framework\Registry mock. I'd rather create that myself and specify it as a constructor argument. That way all dependencies are clear when reading the test code.

Also, since PHP 5.5 the ::class constant is available. This is greatly preferable over using strings for class names, as it aids with automatic refactoring in the IDE and finding usages of a given class. It makes PHPStorm smarter. So I would replace all string class names with the constant, e.g. 'Magento\Framework\App\RequestInterface' with \Magento\Framework\App\RequestInterface::class.

Also, I question the use of \Magento\Framework\TestFramework\Unit\Helper\ObjectManager.
In my opinion it is better to instantiate the class under test manually using new. The only thing the helper does at this time is it creates a \Magento\Framework\Registry mock. I'd rather create that myself and specify it as a constructor argument. That way all dependencies are clear when reading the test code.

The next cleanup is rather important. I would change the unit test methods to notnot mirror the implementation exactly.
For example, take the setup of the request mock in testDeleteActionThrowsException:

Does it really matter how often getParam is called.? Should the test fail if it gets called twice, or not at all? I think that is not important at all, as long as we test the final result of the method is what we expect. Binding
Binding the test code more closely to the implementation then needed leads to rigid tests that are harder to maintain.
So this example I would refactor to

And finally, because expects($this->any()) is the default, it is good to remove that to reduce the amount of clutter.

$this->requestMock->method('getParam')->willReturn($this->blockId);

This ready much nicer.

Arguably it might make sense to specify the expected parameter to getParam in this test, even though the original test author omitted it.

$this->requestMock->method('getParam')
    ->with('block_id')
    ->willReturn($this->blockId);

This is probably how I would leave the test and move on.

One more thought though: the problem with getter methods like getParam is that if a caller tries to access different values, the mock has to return different things based on the argument value.
Such changes in future are quite likely, so sometimes I specify a return value map, even if there is only one value. This makes it easy to maintain the test when the class that is being tested changes in future.

$this->requestMock->method('getParam')
    ->willReturnMap([
        ['block_id', null, $this->blockId]
    ]);

In case you are not familiar with PHPUnit return value maps, the null value in the array is the optional second parameter to getParam($key, $defaultValue = null).

The $objectManagerMock property now is obsolete and all references to it can (actually, should) be removed from the test class.

Next, I question the use of \Magento\Framework\TestFramework\Unit\Helper\ObjectManager.
In my opinion (and that is only me) it is better to instantiate the class under test manually using new. The only thing the helper does at this time is it creates a \Magento\Framework\Registry mock. I'd rather create that myself and specify it as a constructor argument. That way all dependencies are clear when reading the test code.

Also, since PHP 5.5 the ::class constant is available. This is greatly preferable over using strings for class names, as it aids with automatic refactoring in the IDE and finding usages of a given class. It makes PHPStorm smarter. So I would replace all string class names with the constant, e.g. 'Magento\Framework\App\RequestInterface' with \Magento\Framework\App\RequestInterface::class.

The next cleanup is rather important. I would change the unit test methods to not mirror the implementation exactly.
For example, take the setup of the request mock in testDeleteActionThrowsException:

Does it really matter how often getParam is called. Should the test fail if it gets called twice? I think that is not important at all. Binding the test code more closely to the implementation then needed leads to rigid tests that are harder to maintain.
So this example I would refactor to

And finally, because expects($this->any()) is the default, it is good to remove that to reduce the amount of clutter in the test.

$this->requestMock->method('getParam')->willReturn($this->blockId);

The $objectManagerMock property now is obsolete and all references to it can (actually, should) be removed from the test class.

Next, since PHP 5.5 the ::class constant is available. This is greatly preferable over using strings for class names, as it aids with automatic refactoring in the IDE and finding usages of a given class. It makes PHPStorm smarter. So I would replace all string class names with the constant, e.g. 'Magento\Framework\App\RequestInterface' with \Magento\Framework\App\RequestInterface::class.

Also, I question the use of \Magento\Framework\TestFramework\Unit\Helper\ObjectManager.
In my opinion it is better to instantiate the class under test manually using new. The only thing the helper does at this time is it creates a \Magento\Framework\Registry mock. I'd rather create that myself and specify it as a constructor argument. That way all dependencies are clear when reading the test code.

The next cleanup is rather important. I would change the unit test methods to not mirror the implementation exactly.
For example, take the setup of the request mock in testDeleteActionThrowsException:

Does it really matter how often getParam is called? Should the test fail if it gets called twice, or not at all? I think that is not important, as long as we test the final result of the method is what we expect.
Binding the test code more closely to the implementation then needed leads to rigid tests that are harder to maintain.
So this example I would refactor to

And finally, because expects($this->any()) is the default, it is good to remove that to reduce the amount of clutter.

$this->requestMock->method('getParam')->willReturn($this->blockId);

This ready much nicer.

Arguably it might make sense to specify the expected parameter to getParam in this test, even though the original test author omitted it.

$this->requestMock->method('getParam')
    ->with('block_id')
    ->willReturn($this->blockId);

This is probably how I would leave the test and move on.

One more thought though: the problem with getter methods like getParam is that if a caller tries to access different values, the mock has to return different things based on the argument value.
Such changes in future are quite likely, so sometimes I specify a return value map, even if there is only one value. This makes it easy to maintain the test when the class that is being tested changes in future.

$this->requestMock->method('getParam')
    ->willReturnMap([
        ['block_id', null, $this->blockId]
    ]);

In case you are not familiar with PHPUnit return value maps, the null value in the array is the optional second parameter to getParam($key, $defaultValue = null).

1
source | link