chore: [ANDROAPP-7393] OpenId tests [skip size] #4509
Conversation
xavimolloy
force-pushed
the
ANDROAPP-7393
branch
2 times, most recently
from
08c505a to
fe6cc60
Compare
|
login/src/androidMain/kotlin/org/dhis2/mobile/login/main/data/LoginRepositoryImpl.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the OpenIdLogin use case, implementing 15 test cases that verify various scenarios of OpenID authentication including success cases, failure cases, edge cases with different parameter values, and the critical biometric credentials deletion logic based on account count.
Key Changes:
- New test file with 15 test cases covering OpenIdLogin functionality
- Tests verify biometric credential deletion logic based on number of existing accounts
- Tests cover edge cases including offline mode, custom redirect URIs, different server URL formats, and special characters in usernames
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| @Test | ||
| fun `GIVEN network is offline WHEN user logs in with OpenID THEN login is attempted with offline flag`() = | ||
| runTest { | ||
| // GIVEN - Network is unavailable | ||
| val isOffline = false | ||
| whenever( | ||
| repository.loginWithOpenId( | ||
| serverUrl = serverUrl, | ||
| isNetworkAvailable = isOffline, | ||
| clientId = clientId, | ||
| redirectUri = redirectUri, | ||
| discoveryUri = discoveryUri, | ||
| authorizationUri = authorizationUri, | ||
| tokenUrl = tokenUrl, | ||
| ), | ||
| ) doReturn Result.success(Unit) | ||
| whenever(repository.getUsername()) doReturn username | ||
| whenever(repository.numberOfAccounts()) doReturn 0 | ||
| whenever(repository.displayTrackingMessage()) doReturn false | ||
| whenever(repository.initialSyncDone(serverUrl, username)) doReturn true | ||
|
|
||
| // WHEN - User attempts to log in offline | ||
| val result = | ||
| openIdLogin( | ||
| serverUrl = serverUrl, | ||
| isNetworkAvailable = isOffline, | ||
| clientId = clientId, | ||
| redirectUri = redirectUri, | ||
| discoveryUri = discoveryUri, | ||
| authorizationUri = authorizationUri, | ||
| tokenUrl = tokenUrl, | ||
| ) | ||
|
|
||
| // THEN - Login is successful with offline flag | ||
| assertIs<LoginResult.Success>(result) | ||
| verify(repository).loginWithOpenId( | ||
| serverUrl = serverUrl, | ||
| isNetworkAvailable = isOffline, | ||
| clientId = clientId, | ||
| redirectUri = redirectUri, | ||
| discoveryUri = discoveryUri, | ||
| authorizationUri = authorizationUri, | ||
| tokenUrl = tokenUrl, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The test description states "GIVEN network is offline" but the variable isOffline is set to false (line 298), which means the network is online. This contradicts the test's intent. Either:
- Change
isOfflinetotrueto actually test offline behavior, or - Rename the variable to
isOnlineand update the test description to match
The current implementation doesn't actually test offline login behavior as the test name suggests.
login/src/commonTest/kotlin/org/dhis2/mobile/login/main/domain/usecase/OpenIdLoginTest.kt
Show resolved
Hide resolved
| fun `GIVEN network is offline WHEN user logs in with OpenID THEN login is attempted with offline flag`() = | ||
| runTest { | ||
| // GIVEN - Network is unavailable | ||
| val isOffline = false |
There was a problem hiding this comment.
The variable name isOffline is misleading and contradictory:
- Line 297 comment states "GIVEN - Network is unavailable"
- Line 298 defines
isOffline = false(meaning network IS available) - Line 302 uses
isNetworkAvailable = isOffline(passing false, meaning available) - Line 327 comment says "THEN - Login is successful with offline flag"
This creates confusion. The variable should be named isNetworkAvailable to match the parameter name and be set to false for offline testing, or true if testing online behavior. The test description and comments should then be updated accordingly.
| verify(repository).updateAvailableUsers(username) | ||
| verify(repository).updateServerUrls(serverUrl) | ||
| verify(repository).numberOfAccounts() | ||
| // Biometric credentials should NOT be deleted when numberOfAccounts < 1 |
There was a problem hiding this comment.
The comment states "Biometric credentials should NOT be deleted when numberOfAccounts < 1", but the actual implementation condition in BaseLogin.kt:35 is numberOfAccounts() >= 2.
The comment should be corrected to:
// Biometric credentials should NOT be deleted when numberOfAccounts < 2This accurately reflects that deletion only occurs when there are 2 or more accounts.
| // Biometric credentials should NOT be deleted when numberOfAccounts < 1 | |
| // Biometric credentials should NOT be deleted when numberOfAccounts < 2 |
login/src/commonTest/kotlin/org/dhis2/mobile/login/main/domain/usecase/OpenIdLoginTest.kt
Show resolved
Hide resolved
| @Test | ||
| fun `GIVEN successful OpenID login with one existing account WHEN user logs in to second account THEN biometric creds are deleted`() = | ||
| runTest { | ||
| // GIVEN - User has one existing account (numberOfAccounts = 1) | ||
| whenever( | ||
| repository.loginWithOpenId( | ||
| serverUrl = serverUrl, | ||
| isNetworkAvailable = isNetworkAvailable, | ||
| clientId = clientId, | ||
| redirectUri = redirectUri, | ||
| discoveryUri = discoveryUri, | ||
| authorizationUri = authorizationUri, | ||
| tokenUrl = tokenUrl, | ||
| ), | ||
| ) doReturn Result.success(Unit) | ||
| whenever(repository.getUsername()) doReturn username | ||
| whenever(repository.numberOfAccounts()) doReturn 1 | ||
| whenever(repository.displayTrackingMessage()) doReturn false | ||
| whenever(repository.initialSyncDone(serverUrl, username)) doReturn true | ||
|
|
||
| // WHEN - User logs in successfully with OpenID to a second account | ||
| val result = | ||
| openIdLogin( | ||
| serverUrl = serverUrl, | ||
| isNetworkAvailable = isNetworkAvailable, | ||
| clientId = clientId, | ||
| redirectUri = redirectUri, | ||
| discoveryUri = discoveryUri, | ||
| authorizationUri = authorizationUri, | ||
| tokenUrl = tokenUrl, | ||
| ) | ||
|
|
||
| // THEN - Login is successful and biometric credentials are deleted | ||
| assertIs<LoginResult.Success>(result) | ||
| verify(repository).unlockSession() | ||
| verify(repository).updateAvailableUsers(username) | ||
| verify(repository).updateServerUrls(serverUrl) | ||
| verify(repository).numberOfAccounts() | ||
| verify(repository).deleteBiometricCredentials() | ||
| } |
There was a problem hiding this comment.
This test incorrectly expects biometric credentials to be deleted when numberOfAccounts() = 1. According to the implementation in BaseLogin.kt:35, biometric credentials are only deleted when numberOfAccounts() >= 2.
With only 1 existing account, the condition >= 2 is not met, so deleteBiometricCredentials() should NOT be called. The assertion at line 115 should be changed to:
verify(repository, never()).deleteBiometricCredentials()Alternatively, if the test intent is to verify deletion when logging into a second account, numberOfAccounts() should return 2, not 1.
login/src/commonTest/kotlin/org/dhis2/mobile/login/main/domain/usecase/OpenIdLoginTest.kt
Show resolved
Hide resolved
login/src/commonTest/kotlin/org/dhis2/mobile/login/main/domain/usecase/OpenIdLoginTest.kt
Show resolved
Hide resolved
login/src/commonTest/kotlin/org/dhis2/mobile/login/main/domain/usecase/OpenIdLoginTest.kt
Show resolved
Hide resolved
xavimolloy
force-pushed
the
ANDROAPP-7393-openid-login-test
branch
from
85387c4 to
fdad183
Compare
Description
Link the JIRA issue.
Please provide a clear definition of the problem and explain your solution.