Skip to content

Issue/13326 tweak scan state builder tests#14032

Merged
ParaskP7 merged 11 commits intodevelopfrom
issue/13326-scan-builder-tests
Feb 11, 2021
Merged

Issue/13326 tweak scan state builder tests#14032
ParaskP7 merged 11 commits intodevelopfrom
issue/13326-scan-builder-tests

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Feb 11, 2021

Parent #13326

This PR

  • Re-adds tests removed in commit d61a3e8 and tweaks them to fit existing list items structure (0be230e)
  • Rewords test statements to given/when/then format (c370ddc)
  • Adds few missing tests for scan state icon, header and progress bar (da44196, 1599af4, 29d803c)

It might be easier to review them commit by commit.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ashiagr ashiagr added this to the 16.8 milestone Feb 11, 2021
@ashiagr ashiagr self-assigned this Feb 11, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 11, 2021 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ashiagr ashiagr requested a review from ParaskP7 February 11, 2021 07:36
@ashiagr ashiagr marked this pull request as ready for review February 11, 2021 07:38
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 11, 2021 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@ParaskP7 ParaskP7 self-assigned this Feb 11, 2021
ParaskP7
ParaskP7 approved these changes Feb 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @ashiagr !

I have reviewed and tested this PR as per the instructions, everything works as expected, really amazing job, the actual coverage for ScanStateListItemsBuilderTest.kt is 100% atm, really awesome! 🌟 🌟 🌟

I have left only minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

PS: I have noticed that you skipped the then word while writing these test sentences, for example:

  1. Before: given fixing threats state, when items are built, items contain shield warning icon with error color
  2. After: given fixing threats state, when items are built, then items contain shield warning icon with error color
    I am not sure if you did that on purpose or not, if not consider adding the then to have this pattern read as expected.

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Feb 11, 2021

Thanks so much for the review @ParaskP7! Really appreciate your time in reviewing all tests 🙇‍♀️.

I have noticed that you skipped the then word while writing these test sentences.
I am not sure if you did that on purpose or not, if not consider adding the then to have this pattern read as expected.

As discussed on Slack, added then in ad56b9e.

Ready for another look :).

@ParaskP7
Copy link
Copy Markdown
Contributor

ParaskP7 commented Feb 11, 2021 •
edited
Loading

👋 @ashiagr !

Thanks so much for the review @ParaskP7! Really appreciate your time in reviewing all tests 🙇‍♀️.

Some here, I appreciate your time in writing all those test! 🙏

As discussed on Slack, added then in ad56b9e.
Ready for another look :).

LGTM 🌟 I'll go ahead and merge the PR now!

@ParaskP7 ParaskP7 merged commit 86fe518 into develop Feb 11, 2021
@ParaskP7 ParaskP7 deleted the issue/13326-scan-builder-tests branch February 11, 2021 11:31
@ashiagr ashiagr mentioned this pull request Feb 12, 2021
Closed
67 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ParaskP7 ParaskP7 ParaskP7 approved these changes

Assignees

@ashiagr ashiagr

@ParaskP7 ParaskP7

Labels

None yet

Projects

None yet

Milestone

16.8

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @ParaskP7