-
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
PhpUnit 8 Migration - AdminNotification #27521
PhpUnit 8 Migration - AdminNotification #27521
Conversation
Fix library related to unit tests
Hi @ihor-sviziev. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
Update unit tests in AdminNotification module
a6202c9
to
0b2cf65
Compare
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.
In future we can change the scope for Test properties from protected
to private
.
I'm not sure if it's for purpose or accidentally, but you've added changes from other PR.
You can put in the description that this PR would work after merging another (however the changes introduced in files related to AdminNotification are backward compatible, so there's no dependency)
app/code/Magento/AdminNotification/Test/Unit/Model/System/Message/SecurityTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/AdminNotification/Test/Unit/Model/System/Message/SecurityTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/AdminNotification/Test/Unit/Model/System/Message/SecurityTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/AdminNotification/Test/Unit/Model/System/Message/SecurityTest.php
Outdated
Show resolved
Hide resolved
@magento run all tests |
@lbajsarowicz |
Hi @lbajsarowicz, thank you for the review.
|
@lenaorobei seems like we have false positive in static test |
@ihor-sviziev looking into it. |
lib/internal/Magento/Framework/TestFramework/Unit/Listener/GarbageCleanup.php
Outdated
Show resolved
Hide resolved
lib/internal/Magento/Framework/TestFramework/Unit/Listener/GarbageCleanup.php
Show resolved
Hide resolved
Yes, the 3-rd finding looks like false-positive, but it should disappear when first 2 will be fixed. |
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.
Yes, I reviewed them before and was waiting for tests to pass.
We have all green, so my approval is there.
Hi @lbajsarowicz, thank you for the review.
|
Hi @ihor-sviziev, thank you for your contribution! |
Description (*)
This PR updates unit tests in AdminNotification module + contains framework fix from #27519
Note:
We have 2 warnings that can't be fixed because these new methods doesn't exists in phpunit 6:
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist app/code/Magento/AdminNotification/
vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist app/code/Magento/AdminNotification/
Questions or comments
Contribution checklist (*)