[Checkout] Covering the ResetQuoteAddresses by Unit Test #26096

eduard13 · 2019-12-18T13:02:57Z

Description (*)

This PR is improving the test coverage for Checkout module, by covering the Magento\Checkout\Plugin\Model\Quote\ResetQuoteAddresses by Unit Tests.

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

...
...

Questions or comments

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-12-18T13:03:01Z

Hi @eduard13. 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.4-develop instance - deploy vanilla Magento instance

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

magento-engcom-team · 2019-12-18T13:27:38Z

Hi @dmytro-ch, thank you for the review.
ENGCOM-6499 has been created to process this Pull Request
✳️ @dmytro-ch, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

dmytro-ch

Hi @eduard13, could you please fix the static test recommendations?

eduard13 · 2019-12-18T13:29:12Z

Hi @dmytro-ch, already did.
Thank you.

magento-engcom-team · 2019-12-18T15:52:39Z

Hi @dmytro-ch, thank you for the review.
ENGCOM-6499 has been created to process this Pull Request

lbajsarowicz

Please get through the recommendations. In my opinion you tried to cover too much logic in single Unit Test.

lbajsarowicz · 2019-12-18T21:20:59Z

app/code/Magento/Checkout/Test/Unit/Plugin/ResetQuoteAddressesTest.php

+        if ($quoteHasAddresses) {
+            $address = $this->createPartialMock(Address::class, ['getId']);
+
+            $address->expects($this->any())
+                ->method('getId')
+                ->willReturn(1);
+
+            $addresses = [$address];
+
+            $this->quoteMock->expects($this->any())
+                ->method('getAllAddresses')
+                ->will($this->returnValue($addresses));
+
+            $this->quoteMock->expects($this->exactly(count($addresses)))
+                ->method('removeAddress')
+                ->willReturnSelf();
+        } else {
+            $this->quoteMock->expects($this->any())
+                ->method('getAllAddresses')
+                ->willReturn([]);
+        }


Having this conditional logic in Unit Test means that you are trying to do too much inside the single test.

Split that into two tests - for example:

testClearingAddressesSuccessfullyFromEmptyQuoteWithAddress

testClearingAddressesSuccessfullyFromEmptyQuoteWithoutAddress

Moved the logic into a separated test cases. 👌

lbajsarowicz · 2019-12-18T21:21:22Z

app/code/Magento/Checkout/Test/Unit/Plugin/ResetQuoteAddressesTest.php

+
+            $address->expects($this->any())
+                ->method('getId')
+                ->willReturn(1);


Try to avoid Magic Numbers - use private const STUB_ADDRESS_ID instead.

Updated. 👌

lbajsarowicz · 2019-12-18T21:22:18Z

app/code/Magento/Checkout/Test/Unit/Plugin/ResetQuoteAddressesTest.php

+        if ($isVirtualQuote && $extensionAttributes) {
+            $this->extensionAttributesMock->expects($this->any())
+                ->method('getShippingAssignments')
+                ->willReturn([1]);


If that 1 means the same ID that STUB_ADDRESS_ID - put that reference there instead of magic number that has unclear meaning.

Actually no, so I moved it to a const as well.

eduard13 · 2019-12-19T15:40:28Z

Hi @lbajsarowicz, thank you for reviewing this one, could you please review it again?
Thank you.

magento-engcom-team · 2019-12-19T15:57:38Z

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

engcom-Alfa · 2019-12-20T08:34:40Z

QA not applicable

…26096

m2-assistant · 2019-12-24T20:56:45Z

Hi @eduard13, 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.

Covering the ResetQuoteAddresses by Unit Test

f935b9a

eduard13 requested a review from paliarush as a code owner December 18, 2019 13:02

magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Dec 18, 2019

dmytro-ch self-assigned this Dec 18, 2019

dmytro-ch approved these changes Dec 18, 2019

View reviewed changes

ghost added the Progress: ready for testing label Dec 18, 2019

magento-engcom-team added Release Line: 2.4 Component: Checkout labels Dec 18, 2019

dmytro-ch requested changes Dec 18, 2019

View reviewed changes

Fixing static tests

98c9154

ghost added Progress: needs update and removed Progress: ready for testing labels Dec 18, 2019

Fixing static tests

1db525a

dmytro-ch added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: test coverage labels Dec 18, 2019

dmytro-ch approved these changes Dec 18, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Dec 18, 2019

magento deleted a comment from magento-engcom-team Dec 18, 2019

lbajsarowicz suggested changes Dec 18, 2019

View reviewed changes

ghost assigned lbajsarowicz Dec 18, 2019

ghost added Progress: needs update and removed Progress: ready for testing labels Dec 18, 2019

Adjusting the Unit Test

e9939e0

Small adjustments

54fe070

lbajsarowicz approved these changes Dec 19, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Dec 19, 2019

engcom-Alfa self-assigned this Dec 20, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Dec 20, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Dec 20, 2019

engcom-Golf self-assigned this Dec 20, 2019

ghost added Progress: accept and removed Progress: extended testing labels Dec 20, 2019

magento-engcom-team pushed a commit that referenced this pull request Dec 24, 2019

ENGCOM-6499: [Checkout] Covering the ResetQuoteAddresses by Unit Test #…

02176a8

…26096

magento-engcom-team merged commit 54fe070 into magento:2.4-develop Dec 24, 2019

[Checkout] Covering the ResetQuoteAddresses by Unit Test #26096

[Checkout] Covering the ResetQuoteAddresses by Unit Test #26096

Uh oh!

Conversation

eduard13 commented Dec 18, 2019

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Dec 18, 2019

Uh oh!

magento-engcom-team commented Dec 18, 2019

Uh oh!

dmytro-ch left a comment

Choose a reason for hiding this comment

Uh oh!

eduard13 commented Dec 18, 2019

Uh oh!

magento-engcom-team commented Dec 18, 2019

Uh oh!

lbajsarowicz left a comment

Choose a reason for hiding this comment

Uh oh!

lbajsarowicz Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

eduard13 Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

lbajsarowicz Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

eduard13 Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

lbajsarowicz Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

eduard13 Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

eduard13 commented Dec 19, 2019

Uh oh!

magento-engcom-team commented Dec 19, 2019

Uh oh!

engcom-Alfa commented Dec 20, 2019

Uh oh!

m2-assistant bot commented Dec 24, 2019

Uh oh!

Uh oh!