Magento 2: Fixing Call to Undefined Method Mock_BlockFactory_4b440480::create() Test Failures

Question

I recently submitted a pull request to Magento to fix a single instance of an object manager being directly used.

However, Magento's travis unit test run failed with the following error.

PHP Fatal error: Call to undefined method Mock_BlockFactory_4b440480::create() in /home/travis/build/magento/magento2/app/code/Magento/Cms/Controller/Adminhtml/Block/Delete.php on line 39

Based on the travis build, I can't even tell which test failed. I've been able to get a similar (identical?) error locally with a stack trace

PHP Fatal error:  Call to undefined method Mock_BlockFactory_ec77572c::create() in /Users/alanstorm/Documents/github/astorm/magento2/app/code/Magento/Cms/Controller/Adminhtml/Block/Delete.php on line 39
PHP Stack trace:
PHP   1. {main}() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/phpunit:55
PHP   3. PHPUnit_TextUI_Command->run() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/TextUI/Command.php:132
PHP   4. PHPUnit_TextUI_TestRunner->doRun() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/TextUI/Command.php:179
PHP   5. PHPUnit_Framework_TestSuite->run() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:426
PHP   6. PHPUnit_Framework_TestSuite->run() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestSuite.php:675
PHP   7. PHPUnit_Framework_TestCase->run() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestSuite.php:675
PHP   8. PHPUnit_Framework_TestResult->run() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestCase.php:753
PHP   9. PHPUnit_Framework_TestCase->runBare() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestResult.php:686
PHP  10. PHPUnit_Framework_TestCase->runTest() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestCase.php:817
PHP  11. ReflectionMethod->invokeArgs() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestCase.php:951
PHP  12. Magento\Cms\Test\Unit\Controller\Adminhtml\Block\DeleteTest->testDeleteAction() /Users/alanstorm/Documents/github/astorm/magento2/vendor/phpunit/phpunit/src/Framework/TestCase.php:951
PHP  13. Magento\Cms\Controller\Adminhtml\Block\Delete->execute() /Users/alanstorm/Documents/github/astorm/magento2/app/code/Magento/Cms/Test/Unit/Controller/Adminhtml/Block/DeleteTest.php:151

I've been able to narrow the local failure down to this test -- but I'm at a bit of a loss at to what's going on.

My guess is that the test framework has automatically mocked a DI argument for me, but that the automatic mocking is missing the create method. If that's the case, then my actual question is How do I add a mock for a newly injected dependency in Magento's test framework.

However, I've never gone this deep down Magento's testing rabbit hole, so I'm not sure what actually needs to happen here. Can anyone with Magento testing experience set me straight?

Answer 1

The \Magento\Framework\TestFramework\Unit\Helper\ObjectManager isn't able to automatically create a factory mock.
(On a side note, I never use the \Magento\Framework\TestFramework\Unit\Helper\ObjectManager, as I try to keep the amount of magic in unit tests to a minimum.)

The following changes are required to make the test pass:

First, create the mock for the model block factory in the setup:

$this->modelBlockFactoryMock = $this->getMockBuilder(\Magento\Cms\Model\BlockFactory::class)
    ->disableOriginalConstructor()
    ->setMethods(['create'])
    ->getMock();

Or, more compact:

$this->modelBlockFactoryMock = $this->getMock(\Magento\Cms\Model\BlockFactory::class, ['create'], [], '', false);

Second, add the new mock to the controllers constructor arguments:

$this->deleteController = $this->objectManager->getObject(
    'Magento\Cms\Controller\Adminhtml\Block\Delete',
    [
        'context' => $this->contextMock,
        'modelBlockFactory' => $this->modelBlockFactoryMock
    ]
);

Third, update the tests so they no longer expect the Block model to be created by the mock object manager but by the new factory:

Replace

$this->objectManagerMock->expects($this->once())
    ->method('create')
    ->with('Magento\Cms\Model\Block')
    ->willReturn($this->blockMock);

with

$this->modelBlockFactoryMock->expects($this->once())
    ->method('create')
    ->willReturn($this->blockMock);

After this the test passes.

Additional cleaning up

The following points have nothing to do with your question, but I can't resist writing them down anyway.

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:

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

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

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

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 reads 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).