-
Notifications
You must be signed in to change notification settings - Fork 2
fix/authentication/signin-ui-tests : Fix and complete UI tests for SignInScreen #73
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
Conversation
Added `com.linkedin.dexmaker:dexmaker-mockito-inline` dependency to enable Mockito inline for androidTests Fix comes from 2776ce7.
Uncommented the existing (partly wrong) tests, corrected them and created new ones. Used mocking for `NavigationActions` and `AuthViewModel` to be able to check for calls to them. Used a `companion object` to have a reusable valid email and password. The tests now check for: * display of all components * correct errors showing up when invalid attempt (empty email or password) * not trying to log in the user if invalid attempt * valid attempt navigates to next screen * valid attempt logs in the user * click on sign up navigates to SignUp screen
Changed deprecated `ClickableText` to a `Text` with a `clickable` attribute on its modifier. This makes it so that the `signUpHereNavigatesToSignUpScreen` test can detect calls to `navigationActions`
Added a `Row` with content two different `Text`s, once for non-clickable text ("Not registered yet? "), and one for clickable text ("Sign up here!").
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.
There's a couple of comments but are all nitpicks but overall LGTM.
app/src/androidTest/java/com/android/periodpals/ui/authentication/SignInTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/com/android/periodpals/ui/authentication/SignInTest.kt
Show resolved
Hide resolved
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.
Summary
Please see comments.
Important
Code Quality
- LGTM
Functionality
- See comments
Testing
- See comments
Documentation
- LGTM
Suggestions
- Maybe add more tests to cover edge cases, such as network errors during login.
Steps before PR approved
- See comments
app/src/androidTest/java/com/android/periodpals/ui/authentication/SignInTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/com/android/periodpals/ui/authentication/SignInTest.kt
Outdated
Show resolved
Hide resolved
This should be handled at the VM level, so those tests would be out of the scope of this PR. |
Replaced ``when`(navigationActions.currentRoute()).thenReturn(Route.ALERT_LIST) ` with correct return screen (`Screen.SIGN_IN`). Added check for not calling the VM in `emptyPasswordDoesNotCallVM`.
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.
LGTM now
Fix and complete UI tests for SignInScreen
Description
This PR introduces new tests for the SignIn screen and fixes the old ones. It also fixes the problems pointed out by said new tests. It closes issue #71 and fixes part of #63 .
Changes
Added dependencies to make mockito work in androidTests (this was also done in
main
and later merged into this branch, so I'll ignore it in the rest of this PR).Properly mocked
NavigationActions
and added verification of correct navigation. Also added checks for the correct calls to theAuthViewModel
on valid attempts and for no calls to theAuthViewModel
on invalid attempts. Removed irrelevant tests (tested for behavior of VM, not the point here).Invalid attempts are ones which have an empty email or password.
Fixed the problems pointed out by the added tests (two calls to
logInWithEmail
instead of one on valid attempt, call tonavigationAction
not detected when trying to sign up).Files
Added
None
Modified
app/src/androidTest/java/com/android/periodpals/ui/alert/SignInScreenTest.kt
: modified, removed and added testsapp/src/main/java/com/android/periodpals/ui/alert/SignInScreen.kt
: fixed problems pointed out by new testsRemoved
None
Dependencies Added
None
Testing
All UI tests for
SignInScreen
pass. They should be complete enough.