Skip to content

[WIP] Enable v1 signin otp tests - #3066

Draft
antrix1989 wants to merge 6 commits into
devfrom
sedemche/enable_v1_signin_otp_tests
Draft

[WIP] Enable v1 signin otp tests#3066
antrix1989 wants to merge 6 commits into
devfrom
sedemche/enable_v1_signin_otp_tests

Conversation

@antrix1989

Copy link
Copy Markdown
Contributor

PR Checklist (must be completed before review)

  • All tests pass locally
  • PR size is <= 500 LOC per PR Size Check policy
  • PR is independently mergeable (no hidden dependencies)
  • Appropriate reviewers are assigned
  • PR reviewed by code owner (required if Copilot-generated)
  • SME or Senior IC assigned where required

PR Title Format

Required Format: [Keyword1] [Keyword2]: Description

  • Keyword1: major, minor, or patch (case-insensitive)
  • Keyword2: feature, bugfix, engg, or tests (case-insensitive)

Examples:

  • [MAJOR] [Feature]: new API
  • [minor] [bugfix]: fix crash
  • [PATCH] [tests]: add coverage

Proposed changes

Describe what this PR is trying to do.

Type of change

  • Feature work
  • Bug fix
  • Documentation
  • Engineering change
  • Test
  • Logging/Telemetry

Risk

  • High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
  • Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
  • Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

Additional information

Copilot AI review requested due to automatic review settings July 30, 2026 21:12
@antrix1989 antrix1989 changed the title Enable v1 signin otp tests [WIP] Enable v1 signin otp tests Jul 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR primarily re-enables and stabilizes Native Auth E2E sign-in/MFA/JIT scenarios (including OTP-based flows) by removing skips, updating delegate plumbing for MFA auth-methods, and adjusting test infrastructure to better support mail-based OTP retrieval.

Changes:

  • Reduced E2E test retry repetitions in the iOS and macOS .xctestplan files.
  • Updated Native Auth E2E delegate spies and multiple sign-in/MFA/JIT tests to run again (removed XCTSkips) and align with updated MFA delegate signatures.
  • Updated the mail-based OTP retrieval helpers (new account creation helper, longer default expectation timeout, and updated test email domain constant).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
MSAL/test/testplan/MSAL Mac Native Auth E2E Tests.xctestplan Lowers maximum test repetitions for macOS native auth E2E runs.
MSAL/test/testplan/MSAL iOS Native Auth E2E Tests.xctestplan Lowers maximum test repetitions for iOS native auth E2E runs.
MSAL/test/integration/native_auth/end_to_end/sign_in/SignInDelegateSpies.swift Updates spy delegate signature to capture MFA auth methods.
MSAL/test/integration/native_auth/end_to_end/sign_in/MSALNativeAuthSignInUsernameEndToEndTests.swift Re-enables multiple username/OTP sign-in E2E tests by removing skips and adjusting test data usage.
MSAL/test/integration/native_auth/end_to_end/sign_in/MSALNativeAuthSignInUsernameAndPasswordEndToEndTests.swift Re-enables username+password E2E tests and adjusts multi-sign-in scenarios.
MSAL/test/integration/native_auth/end_to_end/otp_code_retriever/MailTMConstants.swift Updates the domain used when generating random sign-up email addresses for OTP flows.
MSAL/test/integration/native_auth/end_to_end/MSALNativeAuthEndToEndBaseTestCase.swift Adds an email-provider account creation helper, updates OTP retrieval signature, and increases default async expectation timeout.
MSAL/test/integration/native_auth/end_to_end/mfa/MSALNativeAuthSignInWithMFAEndToEndTests.swift Re-enables MFA E2E tests and refactors parts of the MFA challenge submission/claims validation flow.
MSAL/test/integration/native_auth/end_to_end/mfa/MSALNativeAuthSignInJITEndToEndTests.swift Re-enables JIT E2E tests and switches to creating authenticated email-provider accounts for OTP-driven steps.

Copilot AI review requested due to automatic review settings July 31, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

MSAL/test/integration/native_auth/end_to_end/mfa/MSALNativeAuthSignInWithMFAEndToEndTests.swift:136

  • Issue: authenticationContextATClaimJson is missing the closing quote after the authenticationContextId.
    Impact: The contains check can become a false positive (e.g., a token containing "acrs":["c41"...] would also match "acrs":["c4).
    Recommendation: Include the closing quote after the interpolated id (you can still omit the closing bracket if you want to allow additional values).
        let authenticationContextId = "c4"
        let authenticationContextRequestClaimJson = "{\"access_token\":{\"acrs\":{\"essential\":true,\"value\":\"\(authenticationContextId)\"}}}"
        let authenticationContextATClaimJson = "\"acrs\":[\"\(authenticationContextId)"

MSAL/test/integration/native_auth/end_to_end/MSALNativeAuthEndToEndBaseTestCase.swift:147

  • Issue: The password parameter is shadowed by a local let password = ... binding.
    Impact: Shadowing makes it harder to reason about which password is being used (call-site override vs. conf.json) and is easy to accidentally break during future edits.
    Recommendation: Use a distinct local name (e.g., providerPassword) and pass that through.
    func retrieveCodeFor(email: String, password: String? = nil) async -> String? {
        guard let password = password ?? retrieveEmailProviderPassword() else {
            XCTFail("email_provider_password not found in conf.json")
            return nil
        }

MSAL/test/integration/native_auth/end_to_end/mfa/MSALNativeAuthSignInWithMFAEndToEndTests.swift:67

  • Issue: The invalid-challenge assertion is commented out, so the test no longer validates what kind of error is produced for an incorrect OTP.
    Impact: This reduces the regression signal: the test can pass even if the SDK returns an unrelated error type/message.
    Recommendation: If isInvalidChallenge is currently not reliable in this environment, keep at least a minimal assertion that an actionable error description is present (or update the expectation once the SDK behavior is fixed).
        XCTAssertTrue(mfaSubmitWrongChallengeDelegateSpy.onMFASubmitChallengeErrorCalled)
        // TODO: we get general error instead of isInvalidChallenge.
//        XCTAssertEqual(mfaSubmitWrongChallengeDelegateSpy.error?.isInvalidChallenge, true)

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