Fix exception handling and cover with unit tests the GetById media asset command #25547

coderimus · 2019-11-10T10:23:23Z

Description (*)

Cover with unit test the GetById command of the Media Gallery functionality. Provide two tests: successful action execution and fail at the not found entity by id.

Fixed Issues (if relevant)

magento/adobe-stock-integration#585

Manual testing scenarios (*)

N/A

Questions or comments

N/A

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2019-11-10T10:23:25Z

Hi @coderimus. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

rogyar · 2019-11-10T13:53:46Z

app/code/Magento/MediaGallery/Test/Unit/Model/Asset/Command/GetByIdTest.php

+        $mediaAssetStub = $this->getMockBuilder(AssetInterface::class)->getMock();
+        $this->assetFactory->expects($this->once())->method('create')->willReturn($mediaAssetStub);
+
+        $this->getMediaAssetById->execute(self::MEDIA_ASSET_STUB_ID);


Please, add an assertion here. The execute method returns a value, so it worth asserting that we have an expected value.

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)

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.

rogyar · 2019-11-10T13:54:37Z

app/code/Magento/MediaGallery/Test/Unit/Model/Asset/Command/GetByIdTest.php

+            ->method('critical')
+            ->willReturnSelf();
+
+        $this->getMediaAssetById->execute(self::MEDIA_ASSET_STUB_ID);


Please, add an assertion of the returned result here as well.

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: $this->expectException(IntegrationException::class); In that case, I think that assertion is satisfied by the fact of throwing an integration exception. What assertion you would like to see here?

I don't think the assertion is necessary here

Agree, missed the point that the test already asserts the exception. Thank you

rogyar · 2019-11-10T13:55:53Z

Also, the execute method has a chance to throw NoSuchEntityException. It would be really great to have one more test that covers a case with throwing the mentioned exception.

coderimus · 2019-11-10T20:58:51Z

@sivaschenko @rogyar
I changed the GetById command: now every step in it has a try-catch block which allows controlling execution in a more logical way. Please, review those changes and covered tets.

But this semantic version test confused me. Do you have any idea why it failed? I didn't change anything according to its report. :(

coderimus · 2019-11-11T07:32:51Z

Nevermind about the semantic version check. I re-run it, and everything is ok now. @rogyar @sivaschenko

sivaschenko

Thanks for the pull request @coderimus ! Please see my review comments

sivaschenko · 2019-11-11T12:19:19Z

app/code/Magento/MediaGallery/Test/Unit/Model/Asset/Command/GetByIdTest.php

+        $mediaAssetStub = $this->getMockBuilder(AssetInterface::class)->getMock();
+        $this->assetFactory->expects($this->once())->method('create')->willReturn($mediaAssetStub);
+
+        $this->getMediaAssetById->execute(self::MEDIA_ASSET_STUB_ID);


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)

sivaschenko · 2019-11-11T12:21:21Z

app/code/Magento/MediaGallery/Test/Unit/Model/Asset/Command/GetByIdTest.php

+
+/**
+ * Test the GetById command model
+ * @SuppressWarnings(PHPMD.CouplingBetweenObjects)


@coderimus can you please remove this warning suppression (and extract some tests to a separate file/class if necessary)

rogyar · 2019-11-13T10:48:25Z

@sivaschenko, what do you think about the proposed approach? It would be really nice if we could prevent copying the setUp method over every test without involving a parent class. However, I don't see many options here (we have discussed a separate, 'context' class, but It was just an idea)

coderimus · 2019-11-13T11:45:27Z

@rogyar you mean that it would be nice to initialise the setUp method at the successful test class and then extend all related test classes from it?

sivaschenko · 2019-11-13T14:07:07Z

@rogyar @coderimus I think in this case there could be unique setUp methods for each test as exception tests should not need all the mocks used in the main case test

coderimus · 2019-11-13T14:49:22Z

@rogyar @sivaschenko
Thank you for your review and comments. I tested both variants: with independent test classes and with the one which has the setup method and all other tests inherited it.

From my perspective, I can say that it will be better to have separate classes. Even with the setup method in most cases similar. In the scope of the Unit testing, this could be possible even better and applicable: we encapsulate the logic of every exception and the logical conclusion it produces. With this, we can avoid a situation when a possible change made for any part of the command can be possibly ignored and doesn't take into account.

About one class with the setup method and its inheritance: this is not so useful for us because unit tests, as I see them, should be as strict as possible to cover existed implementation. Add the inheritance of the setup method results in the fact that we rely on a parent class for tests in all cases and make the unit test structure more complicated.

My latest implementation consists of separate classes. Please, check them when you will have time.

magento-engcom-team · 2019-11-17T21:24:43Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6294 has been created to process this Pull Request

magento-engcom-team · 2019-11-18T16:35:35Z

Hi @rogyar, thank you for the review.
ENGCOM-6294 has been created to process this Pull Request

…#25547

m2-assistant · 2019-11-21T04:45:02Z

Hi @coderimus, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

585: cover with unit test the GetById media asset command

5c3424f

coderimus added the Component: MediaGallery label Nov 10, 2019

coderimus requested a review from sivaschenko November 10, 2019 10:23

magento-engcom-team added Release Line: 2.3 Community Insider: Magecom community-insider-contribution labels Nov 10, 2019

rogyar self-assigned this Nov 10, 2019

rogyar requested changes Nov 10, 2019

View reviewed changes

ghost added the Progress: needs update label Nov 10, 2019

rogyar added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Nov 10, 2019

Change the GetById command execution logic and cover with tests

d08eb44

coderimus force-pushed the 585/cover-with-unit-tests-getbyid-action branch from ea2f050 to d08eb44 Compare November 10, 2019 19:52

coderimus requested a review from rogyar November 10, 2019 20:58

coderimus mentioned this pull request Nov 11, 2019

Cover GetById with a unit test magento/adobe-stock-integration#585

Closed

sivaschenko requested changes Nov 11, 2019

View reviewed changes

ghost assigned sivaschenko Nov 11, 2019

Change the way the GetById command is tested

b0fb363

coderimus force-pushed the 585/cover-with-unit-tests-getbyid-action branch from f224328 to b0fb363 Compare November 12, 2019 16:34

coderimus force-pushed the 585/cover-with-unit-tests-getbyid-action branch from ee76ae8 to 4b03395 Compare November 13, 2019 13:55

Change the way the setup method is called for GetById command test

061d5a3

coderimus force-pushed the 585/cover-with-unit-tests-getbyid-action branch from 4b03395 to 061d5a3 Compare November 13, 2019 14:29

Remove not used const

c2209fb

coderimus requested a review from sivaschenko November 13, 2019 16:24

sivaschenko approved these changes Nov 17, 2019

View reviewed changes

ghost added Progress: ready for testing Progress: accept and removed Progress: needs update Progress: ready for testing labels Nov 17, 2019

sivaschenko added the Award: test coverage label Nov 17, 2019

rogyar approved these changes Nov 18, 2019

View reviewed changes

sivaschenko changed the title ~~585: cover with unit test the GetById media asset command~~ Fix exception handling and cover with unit tests the GetById media asset command Nov 20, 2019

magento-engcom-team pushed a commit that referenced this pull request Nov 21, 2019

ENGCOM-6294: 585: cover with unit test the GetById media asset command …

59dac90

…#25547

magento-engcom-team merged commit c2209fb into magento:2.3-develop Nov 21, 2019

magento-engcom-team added this to the Release: 2.3.5 milestone Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix exception handling and cover with unit tests the GetById media asset command #25547

Fix exception handling and cover with unit tests the GetById media asset command #25547

coderimus commented Nov 10, 2019 •

edited by sivaschenko

Loading

m2-assistant bot commented Nov 10, 2019

rogyar Nov 10, 2019

sivaschenko Nov 11, 2019

rogyar Nov 11, 2019

rogyar Nov 10, 2019

coderimus Nov 10, 2019

sivaschenko Nov 11, 2019

rogyar Nov 11, 2019

rogyar commented Nov 10, 2019

coderimus commented Nov 10, 2019 •

edited

Loading

coderimus commented Nov 11, 2019

sivaschenko left a comment

sivaschenko Nov 11, 2019

sivaschenko Nov 11, 2019

rogyar commented Nov 13, 2019

coderimus commented Nov 13, 2019

sivaschenko commented Nov 13, 2019

coderimus commented Nov 13, 2019

magento-engcom-team commented Nov 17, 2019

magento-engcom-team commented Nov 18, 2019

m2-assistant bot commented Nov 21, 2019

Fix exception handling and cover with unit tests the GetById media asset command #25547

Fix exception handling and cover with unit tests the GetById media asset command #25547

Conversation

coderimus commented Nov 10, 2019 • edited by sivaschenko Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogyar commented Nov 10, 2019

coderimus commented Nov 10, 2019 • edited Loading

coderimus commented Nov 11, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogyar commented Nov 13, 2019

coderimus commented Nov 13, 2019

sivaschenko commented Nov 13, 2019

coderimus commented Nov 13, 2019

magento-engcom-team commented Nov 17, 2019

magento-engcom-team commented Nov 18, 2019

m2-assistant bot commented Nov 21, 2019

coderimus commented Nov 10, 2019 •

edited by sivaschenko

Loading

coderimus commented Nov 10, 2019 •

edited

Loading