Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @ashiagr !
I have reviewed and tested this PR as per the instructions. All looks good to me, nice work, especially with the tests! 🌟
I have only minor test related comment(s). I am going to approve this PR but give you the change to apply any comments before merging this to develop. Feel free to merge it as is if you like.
PS: What would you say if we added a test for the GetThreadModelUseCase? I know this use case is very simple, as it will only verify that the scanStore is triggered, but even that is a valid test IMHO and can help setting up the testing structure so that when in the future this become more complicated the test a) might fail and b) be easier to enhance (since the test suite is already available). Wdyt?
.../test/java/org/wordpress/android/ui/jetpack/scan/details/usecases/IgnoreThreatUseCaseTest.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the review @ParaskP7! You always come up with thoughtful suggestions!
Added to the backlog, will surely consider adding it, thank you! |
You are very kind @ashiagr ! Thank you for considering my suggestions. 🤗 PS: When you find yourself under the clock, feel free to skip any of those. My intention is mainly discussing the subject, whether that is applied or not afterwards is secondary to me.
Thank YOU! ❤️ |
Parent #13768
This PR adds ignore threat use case.
To test:
That
IgnoreThreatUseCaseTesttests run fine and cover action results.PR submission checklist:
RELEASE-NOTES.txtif necessary.