From 3658a1ba09b00111c7563471b23eb05fb2f75a9a Mon Sep 17 00:00:00 2001 From: Alok Patel Date: Tue, 3 Mar 2020 15:21:27 +0530 Subject: [PATCH 1/5] Update AbstractResource.php --- .../Framework/Model/ResourceModel/AbstractResource.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php index 255e6d94d741f..555a73577e6f6 100644 --- a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php +++ b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php @@ -92,12 +92,12 @@ public function commit() */ if ($this->getConnection()->getTransactionLevel() === 0) { $callbacks = CallbackPool::get(spl_object_hash($this->getConnection())); - try { - foreach ($callbacks as $callback) { - call_user_func($callback); - } - } catch (\Exception $e) { + foreach ($callbacks as $callback) { + try { + call_user_func($callback); + } catch (\Exception $e) { $this->getLogger()->critical($e); + } } } return $this; From 2ef402a91a88c098db11a1024940ada4c842d16e Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Mon, 16 Mar 2020 09:47:43 +0200 Subject: [PATCH 2/5] fix static, test coverage --- .../Model/ResourceModel/AbstractResource.php | 12 +- .../ResourceModel/AbstractResourceTest.php | 183 +++++++++++++----- 2 files changed, 138 insertions(+), 57 deletions(-) diff --git a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php index 555a73577e6f6..51a59df3383af 100644 --- a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php +++ b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php @@ -13,6 +13,7 @@ /** * Abstract resource model * + * phpcs:disable Magento2.Classes.AbstractApi * @api */ abstract class AbstractResource @@ -93,13 +94,14 @@ public function commit() if ($this->getConnection()->getTransactionLevel() === 0) { $callbacks = CallbackPool::get(spl_object_hash($this->getConnection())); foreach ($callbacks as $callback) { - try { - call_user_func($callback); - } catch (\Exception $e) { - $this->getLogger()->critical($e); - } + try { + call_user_func($callback); + } catch (\Exception $e) { + $this->getLogger()->critical($e); + } } } + return $this; } diff --git a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php index 11917d4df1973..4f979f554d3ee 100644 --- a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php +++ b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php @@ -3,67 +3,83 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ +declare(strict_types=1); + namespace Magento\Framework\Model\Test\Unit\ResourceModel; use Magento\Framework\DataObject; use Magento\Framework\DB\Adapter\AdapterInterface; -use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; use Magento\Framework\Serialize\Serializer\Json; +use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; +use PHPUnit\Framework\MockObject\MockObject as MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; -class AbstractResourceTest extends \PHPUnit\Framework\TestCase +/** + * Test for \Magento\Framework\Model\ResourceModel\AbstractResource. + */ +class AbstractResourceTest extends TestCase { /** * @var AbstractResourceStub */ - private $abstractResource; + private $model; /** - * @var Json|\PHPUnit_Framework_MockObject_MockObject + * @var Json|MockObject */ private $serializerMock; /** - * @var \Psr\Log\LoggerInterface|\PHPUnit_Framework_MockObject_MockObject + * @var LoggerInterface|MockObject */ private $loggerMock; + /** + * @inheritdoc + */ protected function setUp() { $objectManager = new ObjectManager($this); + $this->model = $objectManager->getObject(AbstractResourceStub::class); $this->serializerMock = $this->createMock(Json::class); - $this->loggerMock = $this->createMock(\Psr\Log\LoggerInterface::class); - $this->abstractResource = $objectManager->getObject(AbstractResourceStub::class); - $objectManager->setBackwardCompatibleProperty($this->abstractResource, 'serializer', $this->serializerMock); - $objectManager->setBackwardCompatibleProperty($this->abstractResource, '_logger', $this->loggerMock); + $this->loggerMock = $this->createMock(LoggerInterface::class); + $objectManager->setBackwardCompatibleProperty($this->model, 'serializer', $this->serializerMock); + $objectManager->setBackwardCompatibleProperty($this->model, '_logger', $this->loggerMock); } /** + * Test fields serialize + * * @param array $arguments - * @param string $expected + * @param string|null $expected * @param array|string|int $serializeCalledWith * @param int $numSerializeCalled + * @return void * @dataProvider serializeFieldsDataProvider */ public function testSerializeFields( array $arguments, - $expected, + ?string $expected, $serializeCalledWith, - $numSerializeCalled = 1 - ) { + int $numSerializeCalled = 1 + ): void { /** @var DataObject $dataObject */ - list($dataObject, $field, $defaultValue, $unsetEmpty) = $arguments; + [$dataObject, $field, $defaultValue, $unsetEmpty] = $arguments; $this->serializerMock->expects($this->exactly($numSerializeCalled)) ->method('serialize') ->with($serializeCalledWith) ->willReturn($expected); - $this->abstractResource->_serializeField($dataObject, $field, $defaultValue, $unsetEmpty); + $this->model->_serializeField($dataObject, $field, $defaultValue, $unsetEmpty); $this->assertEquals($expected, $dataObject->getData($field)); } /** + * DataProvider for testSerializeFields() + * * @return array */ - public function serializeFieldsDataProvider() + public function serializeFieldsDataProvider(): array { $array = ['a', 'b', 'c']; $string = 'i am string'; @@ -75,60 +91,66 @@ public function serializeFieldsDataProvider() 'string' => $string, 'integer' => $integer, 'empty' => $empty, - 'empty_with_default' => '' + 'empty_with_default' => '', ] ); + return [ [ [$dataObject, 'array', null, false], '["a","b","c"]', - $array + $array, ], [ [$dataObject, 'string', null, false], '"i am string"', - $string + $string, ], [ [$dataObject, 'integer', null, false], '969', - $integer + $integer, ], [ [$dataObject, 'empty', null, true], null, $empty, - 0 + 0, ], [ [$dataObject, 'empty_with_default', 'default', false], '"default"', - 'default' - ] + 'default', + ], ]; } /** + * Test fields unserialize + * * @param array $arguments * @param array|string|int|boolean $expected + * @return void * @dataProvider unserializeFieldsDataProvider */ - public function testUnserializeFields(array $arguments, $expected) + public function testUnserializeFields(array $arguments, $expected): void { /** @var DataObject $dataObject */ - list($dataObject, $field, $defaultValue) = $arguments; + [$dataObject, $field, $defaultValue] = $arguments; $this->serializerMock->expects($this->once()) ->method('unserialize') ->with($dataObject->getData($field)) ->willReturn($expected); - $this->abstractResource->_unserializeField($dataObject, $field, $defaultValue); + $this->model->_unserializeField($dataObject, $field, $defaultValue); $this->assertEquals($expected, $dataObject->getData($field)); } /** + * DataProvider for testUnserializeFields() + * * @return array */ - public function unserializeFieldsDataProvider() + public function unserializeFieldsDataProvider(): array { $dataObject = new DataObject( [ @@ -137,54 +159,60 @@ public function unserializeFieldsDataProvider() 'integer' => '969', 'empty_with_default' => '""', 'not_serialized_string' => 'i am string', - 'serialized_boolean_false' => 'false' + 'serialized_boolean_false' => 'false', ] ); + return [ [ [$dataObject, 'array', null], - ['a', 'b', 'c'] + ['a', 'b', 'c'], ], [ [$dataObject, 'string', null], - 'i am string' + 'i am string', ], [ [$dataObject, 'integer', null], - 969 + 969, ], [ [$dataObject, 'empty_with_default', 'default', false], - 'default' + 'default', ], [ [$dataObject, 'not_serialized_string', null], - 'i am string' + 'i am string', ], [ [$dataObject, 'serialized_boolean_false', null], false, - ] + ], ]; } - - public function testCommitZeroLevel() + + /** + * Commit zero level + * + * @return void + */ + public function testCommitZeroLevel(): void { - /** @var AdapterInterface|\PHPUnit_Framework_MockObject_MockObject $connection */ + /** @var AdapterInterface|MockObject $connection */ $connection = $this->createMock(AdapterInterface::class); - /** @var DataObject|\PHPUnit_Framework_MockObject_MockObject $closureExpectation */ + /** @var DataObject|MockObject $closureExpectation */ $closureExpectation = $this->getMockBuilder(DataObject::class) ->disableOriginalConstructor() ->getMock(); - $this->abstractResource->setConnection($connection); - $this->abstractResource->addCommitCallback( + $this->model->setConnection($connection); + $this->model->addCommitCallback( function () use ($closureExpectation) { $closureExpectation->setData(1); } ); - $this->abstractResource->addCommitCallback( + $this->model->addCommitCallback( function () use ($closureExpectation) { $closureExpectation->getData(); } @@ -201,16 +229,21 @@ function () use ($closureExpectation) { $closureExpectation->expects($this->once()) ->method('getData'); - $this->abstractResource->commit(); + $this->model->commit(); } - public function testCommitZeroLevelCallbackException() + /** + * Commit zero level callback with exception + * + * @return void + */ + public function testCommitZeroLevelCallbackException(): void { /** @var AdapterInterface|\PHPUnit_Framework_MockObject_MockObject $connection */ $connection = $this->createMock(AdapterInterface::class); - $this->abstractResource->setConnection($connection); - $this->abstractResource->addCommitCallback( + $this->model->setConnection($connection); + $this->model->addCommitCallback( function () { throw new \Exception(); } @@ -224,20 +257,25 @@ function () { $this->loggerMock->expects($this->once()) ->method('critical'); - $this->abstractResource->commit(); + $this->model->commit(); } - public function testCommitNotCompletedTransaction() + /** + * Commit of transactions that have not been completed + * + * @return void + */ + public function testCommitNotCompletedTransaction(): void { - /** @var AdapterInterface|\PHPUnit_Framework_MockObject_MockObject $connection */ + /** @var AdapterInterface|MockObject $connection */ $connection = $this->createMock(AdapterInterface::class); - /** @var DataObject|\PHPUnit_Framework_MockObject_MockObject $closureExpectation */ + /** @var DataObject|MockObject $closureExpectation */ $closureExpectation = $this->getMockBuilder(DataObject::class) ->disableOriginalConstructor() ->getMock(); - $this->abstractResource->setConnection($connection); - $this->abstractResource->addCommitCallback( + $this->model->setConnection($connection); + $this->model->addCommitCallback( function () use ($closureExpectation) { $closureExpectation->setData(1); } @@ -253,6 +291,47 @@ function () use ($closureExpectation) { ->method('setData') ->with(1); - $this->abstractResource->commit(); + $this->model->commit(); + } + + /** + * Test commit case when first callback throws an exception but other callbacks will be called + * + * @return void + */ + public function testCommitFewCallbacksWithException(): void + { + /** @var AdapterInterface|MockObject $connection */ + $connection = $this->createMock(AdapterInterface::class); + + /** @var DataObject|MockObject $closureExpectation */ + $closureExpectation = $this->getMockBuilder(DataObject::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->model->setConnection($connection); + $this->model->addCommitCallback( + function () { + throw new \Exception(); + } + ); + + $this->model->addCommitCallback( + function () use ($closureExpectation) { + $closureExpectation->getData(); + } + ); + + $connection->expects($this->once()) + ->method('commit'); + $connection->expects($this->once()) + ->method('getTransactionLevel') + ->willReturn(0); + $this->loggerMock->expects($this->once()) + ->method('critical'); + $closureExpectation->expects($this->once()) + ->method('getData'); + + $this->model->commit(); } } From e6c960109cb497280bec1ede0ae0464efa92fad7 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Fri, 8 May 2020 11:39:16 +0300 Subject: [PATCH 3/5] marked method as deprecated --- .../Magento/Framework/Model/ResourceModel/AbstractResource.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php index 51a59df3383af..b55b33ddfa54b 100644 --- a/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php +++ b/lib/internal/Magento/Framework/Model/ResourceModel/AbstractResource.php @@ -82,6 +82,7 @@ public function addCommitCallback($callback) /** * Commit resource transaction * + * @deprecated see \Magento\Framework\Model\ExecuteCommitCallbacks::afterCommit * @return $this * @api */ From f733ee3f6929cc20430d81339d694f7f3cf4fdb7 Mon Sep 17 00:00:00 2001 From: Vadim Malesh <51680850+engcom-Charlie@users.noreply.github.com> Date: Fri, 8 May 2020 12:30:09 +0300 Subject: [PATCH 4/5] fix static --- .../Model/Test/Unit/ResourceModel/AbstractResourceTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php index 4f979f554d3ee..e795e3dc7d40c 100644 --- a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php +++ b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php @@ -239,7 +239,7 @@ function () use ($closureExpectation) { */ public function testCommitZeroLevelCallbackException(): void { - /** @var AdapterInterface|\PHPUnit_Framework_MockObject_MockObject $connection */ + /** @var AdapterInterface|MockObject $connection */ $connection = $this->createMock(AdapterInterface::class); $this->model->setConnection($connection); From e28686ffb19860245eee734914a04c15ecb84765 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Wed, 13 May 2020 16:47:35 +0300 Subject: [PATCH 5/5] fixed unit test --- .../ResourceModel/AbstractResourceTest.php | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php index 9b8421bddf9fd..f3b8bb74737b7 100644 --- a/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php +++ b/lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/AbstractResourceTest.php @@ -15,6 +15,9 @@ use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +/** + * Test for \Magento\Framework\Model\ResourceModel\AbstractResource. + */ class AbstractResourceTest extends TestCase { /** @@ -32,16 +35,17 @@ class AbstractResourceTest extends TestCase */ private $loggerMock; + /** + * @inheritdoc + */ protected function setUp(): void - { $objectManager = new ObjectManager($this); - $this->model = $objectManager->getObject(AbstractResourceStub::class); $this->serializerMock = $this->createMock(Json::class); $this->loggerMock = $this->getMockForAbstractClass(LoggerInterface::class); - $this->abstractResource = $objectManager->getObject(AbstractResourceStub::class); - $objectManager->setBackwardCompatibleProperty($this->abstractResource, 'serializer', $this->serializerMock); - $objectManager->setBackwardCompatibleProperty($this->abstractResource, '_logger', $this->loggerMock); + $this->model = $objectManager->getObject(AbstractResourceStub::class); + $objectManager->setBackwardCompatibleProperty($this->model, 'serializer', $this->serializerMock); + $objectManager->setBackwardCompatibleProperty($this->model, '_logger', $this->loggerMock); } /** @@ -187,8 +191,12 @@ public function unserializeFieldsDataProvider(): array ]; } - - public function testCommitZeroLevel() + /** + * Commit zero level + * + * @return void + */ + public function testCommitZeroLevel(): void { /** @var AdapterInterface|MockObject $connection */ $connection = $this->getMockForAbstractClass(AdapterInterface::class);