Android E2E Sign Up, Fixes AB#3073857 #2213

Yuki-YuXin · 2024-10-31T15:07:14Z

Create a draft version instead of an official one: #2212 because the PR is still in the draft version.

Android Test plan: https://identitydivision.visualstudio.com/Engineering/_testPlans/define?planId=3054662&suiteId=3054727
2.1.5
2.1.6
2.1.7 (out of scopes for the Lab environment, include them once the environment is ready)
2.1.8
2.1.9
2.1.10
2.1.11

1.1.2
1.1.5
1.1.10
1.1.11
1.1.12
1.1.13
1.1.14

…pplicationTest using mock API)

… allow flexibility. Complete sign up email otp case

# Conflicts: # msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

Yuki-YuXin · 2024-11-20T11:13:11Z

nilo-ms · 2024-11-20T14:47:55Z

.../client/e2e/tests/network/nativeauth/NativeAuthPublicClientApplicationAnotherAbstractTest.kt

+import org.robolectric.annotation.Config
+import org.robolectric.annotation.LooperMode
+
+// TODO: move to "PAUSED". A work in RoboTestUtils will be needed though.


Is this TODO needed?

Removing this TODO is not inside the work of this scope thus I would be prudential here of removing it.

.../client/e2e/tests/network/nativeauth/NativeAuthPublicClientApplicationAnotherAbstractTest.kt

nilo-ms · 2024-11-20T14:51:52Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInEmailPasswordTest.kt

@@ -85,8 +85,8 @@ class SignInEmailPasswordTest : NativeAuthPublicClientApplicationAbstractTest()
        Assert.assertTrue((result as SignInError).isInvalidCredentials())
    }

-    @Test
    @Ignore("Ignore until MFA is available on test slice")


Can we remove this "Ignore" line? It should be available now

Removing this tag causes failure in the pipeline. I may need to re-add that.

MFA is available for the lab tenant. What the pipeline error is about?

"Error: invalid_grant - AADSTS9002313: Invalid request. Request is malformed or invalid. Trace ID: 567dd14e-a142-4138-83d4-056260310100 Correlation ID: 48bd9ed2-491d-46cd-ba3a-83f32d6e23f2 Timestamp: 2024-11-20 16:50:25Z"

Can you try to run the test locally and fix it? From the code it seems the response is being mocked, so this test will probably need to be updated to work with the real API in the same line as the tests you've added in the PR.

For reference, this is the test file on iOS: https://github.com/AzureAD/microsoft-authentication-library-for-objc/blob/dev/MSAL/test/integration/native_auth/end_to_end/mfa/MSALNativeAuthSignInWithMFAEndToEndTests.swift

Hi @nilo-ms @diegojerezba . Under SignInMFATest, there is a test case called fun test get other auth methods, request challenge on specific auth method and complete MFA flow(). It overlaps with this one under SignInEmailPasswordTest, so I'll remove this test case

OK, but the tests in SignInMFATest are all disabled. Can you enable them?

nilo-ms · 2024-11-20T15:03:03Z

...c/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailOTPTest.kt

+            val signUpResult = application.signUp(user)
+            assertResult<SignUpResult.CodeRequired>(signUpResult)
+            val codeRequiredState = (signUpResult as SignUpResult.CodeRequired).nextState
+            val resendCodeResult = codeRequiredState.resendCode()


To test that the resend code works properly we should:

signUp

retrieve code from mailbox (otp1)

resend code

retrieve code from mailbox (otp2)

check that otp1 and otp2 are different

Same comment applies also for use case 1.1.2

https://identitydivision.visualstudio.com/Engineering/_workitems/edit/2702377 According to the steps defined in the test case, SignInResendCodeResult.Success would be better than otp1 and otp2 difference.

based on the test case you linked:

You need to check that a new code is received in the mailbox. Reviewing your code, it seems that this step is missing. Do you agree?
You decide the best way to check if a new code has been received.

SDK.resendCode() action then SDK return SignInResendCodeResult.Success Result. Receive a new code in the email box is the UI appearance. E2E test is focused on the SDK test without any UI requirement.

I think Yuki we're not understanding each other. Let's have a call and I can explain you what I mean

Me and Yuki had a chat, but we couldn't reach an agreement. Explaining here my reasoning why this test is missing a crucial piece:

E2E tests are meant to validate the real product in a production environment. No component should be mocked. These tests should check that the entire product works as intended. In this case we want to test that the SDK resendCode feature works properly. This means, that when the sdk.resendCode() method is called, we need to check that the SDK returns a successful result and that a new email containing the OTP code is received in the email box.

Additionally, we're automating the manual test (https://identitydivision.visualstudio.com/Engineering/_workitems/edit/2702377). scenario 1.1.2 from acceptance criteria. One step of the test requires verifying that a new code is sent to the email box. When automating this test, we should include all steps.

That's a good point. Done.

I agree with @nilo-ms. Checking if the code has arrived in the mailbox should be included in our logic.

The E2E test simulates what a real user would do. A user would first check if the code has arrived before doing the next step.

Also, as we’ve seen before, the main reason our tests are unreliable is that emails don’t always arrive on time. If we check for the email before moving to the next step, it will make it easier to understand why the test is failing (we can log this information to the console).

@Yuki-YuXin I see that you've implemented this in the test case 2.1.5, but not in the 1.1.2. Can you please implement it there as well? I will resolve this conversation afterwards.

My mistake. Update the 1.1.2 as well.

…p-e2e

…lisation outside

...dentity/client/e2e/tests/network/nativeauth/NativeAuthPublicClientApplicationAbstractTest.kt

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInEmailPasswordTest.kt

...c/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailOTPTest.kt

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

diegojerezba · 2024-11-27T12:54:11Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInEmailPasswordTest.kt

+            // Turn an existing username to a non-existing username
+            val alteredUsername = username.replace("@", "1234@")
+            val result = application.signIn(alteredUsername, password.toCharArray())
+            Assert.assertTrue(result is SignInError)


Nit: these Assert. are redundant and can be removed, in the same style as the test testErrorIsInvalidCredentials() (there are several in the PR)

diegojerezba · 2024-11-27T14:02:48Z

...c/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailOTPTest.kt

+     * (Test case 13)
+     */
+    @Test
+    fun testErrorUserExistAsPassword() {


This test doesn't belong here, and it already exists in SignUpEmailPasswordTests (test 1.1.10) so I think you can remove it.

test 1.1.10 is email+password authentication and this one is using email+otp authentication.

There is a test case in the Android board there we need to run it manually.

https://microsofteur-my.sharepoint.com/:w:/g/personal/sodenhoven_microsoft_com/ERrvxU_ZybBIvnq1UfVKWagBG-u_xFS7pcvRbVXCzFHnsQ?e=5EAUlc It was not included in the numbering of the Native Auth Acceptance criteria, so it was added to the Test plan later

diegojerezba · 2024-11-27T14:06:55Z

...c/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailOTPTest.kt

+    @Ignore("TODO: Add social account in the tenant.")
+    @Test
+    fun testErrorUserExistAsSocial() {
+        config = getConfig(ConfigType.SIGN_IN_PASSWORD)


Nit: it doesn't really matter, but this test is defined as an OTP test in the test plan.

Done. Use ConfigType.SIGN_IN_OTP instead.

diegojerezba · 2024-11-27T14:11:47Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignInEmailPasswordTest.kt

+            val username = config.email
+            val password = getSafePassword()
+            // Turn an existing username to a non-existing username
+            val alteredUsername = username.replace("@", "1234@")


Nit: Is it possible to use the INVALID_EMAIL from NativeAuthPublicClientApplicationAbstractTest to keep consistency with other tests? (For example, it is used in SignUpEmailOTPTest.testErrorInvalidEmailFormat())

Same in line 184 with the alteredPassword.

diegojerezba · 2024-11-27T14:32:40Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

Nit: Can you put the tests of this file in order? This helps a lot while reading them for first time.

This is what I mean:

Test 1.1.1
Test 1.1.2
Test 1.1.4
...

Done on both.

Also, according to Silviu's suggestions, I may remove all the Test case number for clarity.

Just in case I am talking about the order here, not Number SO 1.1.3 is ok, 13 is not

Test plan 13 is a special one that does not exist in our original user case sequence, but was added as a patch to the test plan at a later stage. At the moment I have removed all Test plan numbers and sorted them by user case.

diegojerezba · 2024-11-27T14:39:18Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

+
+        runBlocking { // Running with runBlocking to avoid default 10 second execution timeout.
+            val user = config.email
+            val signUpResult = application.signUp(user)


Password is missing.

Same in tests 1.1.2, 1.1.11 and 1.1.12

Nice catch. Done.

diegojerezba · 2024-11-27T14:41:31Z

...t/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

+            assertResult<SignUpResult.CodeRequired>(signUpResult)
+            val codeRequiredState = (signUpResult as SignUpResult.CodeRequired).nextState
+            val resendCodeResult = codeRequiredState.resendCode()
+            assertResult<SignUpResendCodeResult.Success>(resendCodeResult)


I know this test is disabled for now, but it should result in userAlreadyExists error.

…p-e2e

…A test

Yuki-YuXin added 6 commits October 23, 2024 16:03

Apply LoggerCheckHelper.kt (applied for those NativeAuthPublicClientA…

44afe84

…pplicationTest using mock API)

Update the NativeAuthPublicClientApplicationAbstractTest structure to…

17f6a07

… allow flexibility. Complete sign up email otp case

Finish sign up otp

3a1df81

Merge branch 'dev' into yuki/signup-e2e

b1150ff

# Conflicts: # msal/src/test/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/SignUpEmailPasswordTest.kt

Remove ignore tag

18326e3

Comment out the logger check

839611a

github-actions bot added the msal label Oct 31, 2024

Yuki-YuXin added 8 commits October 31, 2024 15:55

Clean up the name

45b81a6

Remove logger

145ff57

Add loggerCheckHelper

122b73c

loggerCheck = setupLoggerCheckHelper()

43f9339

Double checking the work scopes have been all included

db78d7f

Remove LoggerCheckHelper.kt experiment since it is out of scope

a2da64d

Fix SignUpEmailPasswordTest.kt testSignInAfterSignUp()

7178cec

Remove LoggerCheckHelper.kt

e897b06

Yuki-YuXin marked this pull request as ready for review November 20, 2024 11:10

Yuki-YuXin requested a review from a team as a code owner November 20, 2024 11:10

Merge branch 'dev' into yuki/signup-e2e

627ef91

nilo-ms reviewed Nov 20, 2024

View reviewed changes

.../client/e2e/tests/network/nativeauth/NativeAuthPublicClientApplicationAnotherAbstractTest.kt Outdated Show resolved Hide resolved

nilo-ms reviewed Nov 20, 2024

View reviewed changes

Yuki-YuXin and others added 7 commits November 20, 2024 16:38

Remove @ignore MFA

91bac62

Merge remote-tracking branch 'origin/yuki/signup-e2e' into yuki/signu…

4b8c24c

…p-e2e

Re-add the ignore tag because of the failure

6a1710d

Address testResendCode comment and situation

242b66c

ALl test cases apply new AbstractTest to extract configuration initia…

bd75314

…lisation outside

Merge branch 'dev' into yuki/signup-e2e

6027036

Remove overlap MFA test case

c2e96b6

Yuki-YuXin and others added 2 commits November 26, 2024 14:20

Add config modification for the testSuccess, which causes the failure

38f1f2d

Merge branch 'dev' into yuki/signup-e2e

adc64bc

spetrescu84 reviewed Nov 27, 2024

View reviewed changes

diegojerezba reviewed Nov 27, 2024

View reviewed changes

Yuki-YuXin added 2 commits November 27, 2024 16:09

Address comments

fa13a6a

Merge remote-tracking branch 'origin/yuki/signup-e2e' into yuki/signu…

5abc55e

…p-e2e

spetrescu84 approved these changes Nov 27, 2024

View reviewed changes

Use SIGN_IN_MFA_MULTI_AUTH config instead of SINGLE for the failed MF…

4cfcce0

…A test

Android E2E Sign Up, Fixes AB#3073857 #2213

Are you sure you want to change the base?

Android E2E Sign Up, Fixes AB#3073857 #2213

Conversation

Yuki-YuXin commented Oct 31, 2024 • edited Loading

Yuki-YuXin commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuki-YuXin Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilo-ms Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuki-YuXin commented Oct 31, 2024 •

edited

Loading

Yuki-YuXin Nov 26, 2024 •

edited

Loading

nilo-ms Nov 20, 2024 •

edited

Loading