-
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
[GiftMessage] Cover Observer SalesEventOrderItemToQuoteItemObserver by Unit Test #26082
[GiftMessage] Cover Observer SalesEventOrderItemToQuoteItemObserver by Unit Test #26082
Conversation
Hi @edenduong. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
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.
I'm really glad with the progress that you've done so far.
Actually - Unit Tests should not contain extra conditional logic.
app/code/Magento/GiftMessage/Test/Unit/Observer/SalesEventOrderItemToQuoteItemObserverTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/GiftMessage/Test/Unit/Observer/SalesEventOrderItemToQuoteItemObserverTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/GiftMessage/Test/Unit/Observer/SalesEventOrderItemToQuoteItemObserverTest.php
Outdated
Show resolved
Hide resolved
Hi @lbajsarowicz : It is ok now. Thank you for your feedback. Please check it again. |
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.
Small things that you may change, but it's not crucial.
Once again great contribution. Thank you!
$this->eventMock->expects($this->atLeastOnce()) | ||
->method('getOrderItem') | ||
->willReturn($this->orderItemMock); | ||
|
||
$this->orderItemMock->expects($this->atLeastOnce()) | ||
->method('getOrder') | ||
->willReturn($this->orderMock); | ||
|
||
$this->observerMock->expects($this->atLeastOnce()) | ||
->method('getEvent') | ||
->willReturn($this->eventMock); |
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.
For these three, I'd avoid ->expects
part, as someone could want to test the scenario where code does not reach the getOrderItem
or getOrder
point.
/** | ||
* Test when the order is reorder | ||
*/ | ||
public function testReorder() |
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.
You can do it even more explanatory: testReorderDoesNotCheckIsMessageAllowed
/** | ||
* Test when the order is new reorder and gift message is not allowed | ||
*/ | ||
public function testNewOrderWhenGiftMessageIsNotAllowed() |
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.
Also: testNewOrderWhenGiftMessageIsNotAllowedDoesNotGetQuoteItem
to explain what actually is going on inside (It's very useful when you're browsing ouput of phpunit execution)
/** | ||
* Test when the order is new reorder and gift message is allowed | ||
*/ | ||
public function testNewOrderWhenGiftMessageIsAllowed() |
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.
testNewOrderWhenGiftMessageIsAllowedCreatesNewMessage
Hi @lbajsarowicz, thank you for the review.
|
✔️ QA Passed |
@lbajsarowicz @engcom-Charlie: Could you please add "Test Coverage Award" for this PR :) Thanks :D |
Sure. I was not into these awards yet. 👍 |
…temObserver by Unit Test #26082
Hi @edenduong, thank you for your contribution! |
Description (*)
Increase Test Coverage by Cover class Magento\GiftMessage\Observer\SalesEventOrderItemToQuoteItemObserver
Fixed Issues (if relevant)
Manual testing scenarios (*)
vendor/bin/phpunit app/code/Magento/GiftMessage/Test/Unit/Observer/SalesEventOrderItemToQuoteItemObserverTest.php
Questions or comments
Contribution checklist (*)