Hani/ Refactor Password Manager

[sv-hyacoub]

Relevant Links

Bugzilla: 1953258

Description of Code / Doc Changes

• Move explicit waits to the POM or BOM classes.
• Move context switch to the POM or BOM classes.

Process Changes Required

Mark the relevant boxes:

• Adds a dependency (rerun pipenv install)
• Changes the BasePage
• Changes or creates a BOM/POM (name the object model): _
• Changes CI flow
• Changes scheduled Beta or DevEdition
• Changes Git hooks or Github settings
• Changes L10n harness

Screenshots or Explanations

If you need to explain your code, do it here.

Comments or Future Work

Do we need to start another PR soon to address something you saw while working on this?

Workflow Checklist

• Please request reviewers
• If this is an unblocker, please post in Slack.
• If asked to address comments, please resolve conversations.
• If asked to change code, please re-request review from the person who wanted changes.

Thank you!

[soncuteanca]

Nice work touching so many files, this a very long suite with many interactions, not sure how much to go into the refactoring process to find a balance between time, effort and possible breakage. Overall it looks good. A few suggestions you might consider:

• method creation for repeated code (eg. add login - to something like about_logins.add_login(origin, username, password), primary password, export passwords)
• add some docstrings for the arguments (eg. in the added verify methods)
• avoid using context managers in the tests - (eg: test_auto_saved_generated_password_context_menu, I think they are only 2 tests)
• as much as possible, handle waits within POM/BOM
• maybe move repeated assertions into POM/BOM or helper methods for readability and reuse (eg. there are quite a lot of inline assertion)
• reduce direct element key usage and hardcoded values in tests

[sv-hyacoub]

Nice work touching so many files, this a very long suite with many interactions, not sure how much to go into the refactoring process to find a balance between time, effort and possible breakage. Overall it looks good. A few suggestions you might consider:

• method creation for repeated code (eg. add login - to something like about_logins.add_login(origin, username, password), primary password, export passwords)
• add some docstrings for the arguments (eg. in the added verify methods)
• avoid using context managers in the tests - (eg: test_auto_saved_generated_password_context_menu, I think they are only 2 tests)
• as much as possible, handle waits within POM/BOM
• maybe move repeated assertions into POM/BOM or helper methods for readability and reuse (eg. there are quite a lot of inline assertion)
• reduce direct element key usage and hardcoded values in tests

Thanks for the detailed review.
Some of the changes were not made due to their complexity, and others will be handled in future pull requests.

[soncuteanca]

Approved based on your comment.