-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exception handling and cover with unit tests the GetById media asset command #25547
Changes from 1 commit
5c3424f
d08eb44
b0fb363
061d5a3
c2209fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Magento\MediaGallery\Test\Unit\Model\Asset\Command; | ||
|
||
use Magento\Framework\App\ResourceConnection; | ||
use Magento\Framework\DB\Adapter\AdapterInterface; | ||
use Magento\Framework\DB\Select; | ||
use Magento\Framework\Exception\IntegrationException; | ||
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; | ||
use Magento\MediaGallery\Model\Asset\Command\GetById; | ||
use Magento\MediaGalleryApi\Api\Data\AssetInterface; | ||
use Magento\MediaGalleryApi\Api\Data\AssetInterfaceFactory; | ||
use PHPUnit\Framework\MockObject\MockObject; | ||
use PHPUnit\Framework\TestCase; | ||
use Psr\Log\LoggerInterface; | ||
use Zend\Db\Adapter\Driver\Pdo\Statement; | ||
|
||
/** | ||
* Test the GetById command model | ||
*/ | ||
class GetByIdTest extends TestCase | ||
{ | ||
private const MEDIA_ASSET_STUB_ID = 1; | ||
|
||
private const MEDIA_ASSET_DATA = ['id' => 1]; | ||
|
||
/** | ||
* @var GetById|MockObject | ||
*/ | ||
private $getMediaAssetById; | ||
|
||
/** | ||
* @var AssetInterfaceFactory|MockObject | ||
*/ | ||
private $assetFactory; | ||
|
||
/** | ||
* @var AdapterInterface|MockObject | ||
*/ | ||
private $adapter; | ||
|
||
/** | ||
* @var Select|MockObject | ||
*/ | ||
private $selectStub; | ||
|
||
/** | ||
* @var Statement|MockObject | ||
*/ | ||
private $statementMock; | ||
|
||
/** | ||
* @var LoggerInterface|MockObject | ||
*/ | ||
private $logger; | ||
|
||
/** | ||
* Initialize basic test class mocks | ||
*/ | ||
protected function setUp(): void | ||
{ | ||
$resourceConnection = $this->createMock(ResourceConnection::class); | ||
$this->assetFactory = $this->createMock(AssetInterfaceFactory::class); | ||
$this->logger = $this->createMock(LoggerInterface::class); | ||
|
||
$this->getMediaAssetById = (new ObjectManager($this))->getObject( | ||
GetById::class, | ||
[ | ||
'resourceConnection' => $resourceConnection, | ||
'assetFactory' => $this->assetFactory, | ||
'logger' => $this->logger, | ||
] | ||
); | ||
$this->adapter = $this->createMock(AdapterInterface::class); | ||
$resourceConnection->method('getConnection')->willReturn($this->adapter); | ||
|
||
$this->selectStub = $this->createMock(Select::class); | ||
$this->selectStub->method('from')->willReturnSelf(); | ||
$this->selectStub->method('where')->willReturnSelf(); | ||
$this->adapter->method('select')->willReturn($this->selectStub); | ||
|
||
$this->statementMock = $this->getMockBuilder(\Zend_Db_Statement_Interface::class)->getMock(); | ||
} | ||
|
||
/** | ||
* Test successful get media asset by id command execution. | ||
*/ | ||
public function testSuccessfulGetByIdExecution(): void | ||
{ | ||
$this->statementMock->method('fetch')->willReturn(self::MEDIA_ASSET_DATA); | ||
$this->adapter->method('query')->willReturn($this->statementMock); | ||
|
||
$mediaAssetStub = $this->getMockBuilder(AssetInterface::class)->getMock(); | ||
$this->assetFactory->expects($this->once())->method('create')->willReturn($mediaAssetStub); | ||
|
||
$this->getMediaAssetById->execute(self::MEDIA_ASSET_STUB_ID); | ||
} | ||
|
||
/** | ||
* Test case when there is no found media asset by id. | ||
*/ | ||
public function testNotFoundMediaAssetException(): void | ||
{ | ||
$this->statementMock->method('fetch')->willReturn([]); | ||
$this->adapter->method('query')->willReturn($this->statementMock); | ||
|
||
$this->expectException(IntegrationException::class); | ||
$this->logger->expects($this->once()) | ||
->method('critical') | ||
->willReturnSelf(); | ||
|
||
$this->getMediaAssetById->execute(self::MEDIA_ASSET_STUB_ID); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, add an assertion of the returned result here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @rogyar , thank you for your review. I have a question about this: do we need to have an assertion here when we already expect an exception: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the assertion is necessary here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, missed the point that the test already asserts the exception. Thank you |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add an assertion here. The
execute
method returns a value, so it worth asserting that we have an expected value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rogyar I don't think the assertion is necessary here as execute method is not supposed to return anything in this specific test (it is supposed to throw an exception instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sivaschenko. In this particular case, we test the positive scenario with no exception. As a result, the
execute
method should return an ID. An assertion of the mentioned ID might be useful here.Probably, I've missed some points.
Thank you.