-
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
Cover CartTotalRepositoryPlugin by unit test and correct docblock #26619
Cover CartTotalRepositoryPlugin by unit test and correct docblock #26619
Conversation
Hi @mrtuvn. 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.
Fix static tests.
@lbajsarowicz i have checked and fix for static tests |
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.
Minor changes to be introduced.
app/code/Magento/Multishipping/Model/Cart/CartTotalRepositoryPlugin.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Multishipping/Test/Unit/Model/Cart/CartTotalRepositoryPluginTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Multishipping/Test/Unit/Model/Cart/CartTotalRepositoryPluginTest.php
Outdated
Show resolved
Hide resolved
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.
@lbajsarowicz can you recheck this
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.
Good job.
Really last thing to finish.
app/code/Magento/Multishipping/Test/Unit/Model/Cart/CartTotalRepositoryPluginTest.php
Outdated
Show resolved
Hide resolved
Fix static tests Update commit with meaningful test name Remove unnecessary assignment stub in test code
5446a0e
to
ad8fea6
Compare
@lbajsarowicz can we have new process on this PR. Still one failed functional tests ! |
@lbajsarowicz can you review last changes 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.
✔️
Hi @lbajsarowicz, thank you for the review.
|
@engcom-Golf Could you explain why it was working before, but after these changes, we need to add View Model? |
@lbajsarowicz bug found by QA process, issue exist in dev branch |
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.
OK. Green from me, then.
Hi @lbajsarowicz, thank you for the review. |
✔️ QA Passed |
Hi @mrtuvn, thank you for your contribution! |
Description (*)
Cover the Plugin CartTotalRepositoryPlugin by Unit Test
correct docblock origin plugin class
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)