Skip to content

US1955755: add refactored tests under new card config integration tes…#230

Open
TyreseWp wants to merge 10 commits intomasterfrom
US1955755_refactor_ui_test_framework
Open

US1955755: add refactored tests under new card config integration tes…#230
TyreseWp wants to merge 10 commits intomasterfrom
US1955755_refactor_ui_test_framework

Conversation

@TyreseWp
Copy link
Contributor

@TyreseWp TyreseWp commented Aug 8, 2024

What

  • Refactored tests under from CardConfigurationIntegrationTest in new class NewCardConfigurationIntegrationTest to not depend on Activity context as it goes against Google's guidance for UI testing
  • Omitted the need to call return this as it returns a static reference to that Java class which made refactoring more difficult when amending class names. Scope functions help negate this
  • Only exposed activity context when necessary
  • Used ViewMatchers from the Espresso context instead of ViewMatchers from the Main Activity context

Why

We should strive to align the codebase with Google's guidance to ensure we can minimise pain when updating dependencies and to avoid pain when refactoring the code base in future

Rally Link

@TyreseWp TyreseWp requested a review from ochalet-wp August 8, 2024 09:25
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is asserting can we rename it so it conveys that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was inherited from the class that already exists in master, I will rename to waitForAssertion()

Copy link
Contributor

Choose a reason for hiding this comment

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

paymentsCvcSessionCheckedState() : let's stick to using the terminology paymentsCvcSwitch and also let's try to keep the naming aligned with other functions that assert things/states and are suffixed with Is or Are, e.g. paymentsCvcSwitchIs(checked/on = false) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to refactor the method name, it was inherited from the class that already exists in master

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything in the body of the function that checks that something is in error state? Are we missing something or does the function need renaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is inherited from the class that already exists in master to avoid any regression

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the arguments on the same line, like other functions, so we are consistent in our code style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure I will make that change

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment can be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

successfulResponse is misleading because it seems to pertain to the discovery. Can we rename AndReturnSuccessfulResponse maybe AndSuccessfullyCreatesSession?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as in class above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method name was inherited from the class that already exists in master

@TyreseWp
Copy link
Contributor Author

Adding screenshot of a local execution to show newly added tests pass in suite

Screenshot 2024-08-12 at 10 36 18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments