-
Notifications
You must be signed in to change notification settings - Fork 152
Re-enable Native Auth SSPR tests with Mail.tm, Fixes AB#3729032 #2553
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
base: dev
Are you sure you want to change the base?
Changes from all commits
a4b718a
4ad73aa
e9d6059
dd6b5ce
ce92902
a6aba48
a90fc17
f56e844
e94d321
3c4a8e3
e1124b5
97dcc4b
f259295
97f1df6
41b12b8
9686b50
e356e84
a599345
4bd47e5
62183f7
6bbd3b1
a1be871
03d18c5
e05aa50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,59 @@ stages: | |
| tasks: msal:jacocoTestReport -PcodeCoverageEnabled=true -ProbolectricSdkVersion=${{variables.robolectricSdkVersion}} -PmockApiUrl=$(MOCK_API_URL) -PnativeAuthConfigString=$(NATIVE_AUTH_CONFIG_STRING) | ||
| javaHomeSelection: $(BuildParameters.javaHomeSelection) | ||
| jdkVersion: 1.17 | ||
| - bash: | | ||
| set -euo pipefail | ||
| SKIP_LABEL="skip-native-auth-e2e-tests" | ||
| # AGENT_TEMPDIRECTORY is a Windows path on the windows-2022 pool; normalise the | ||
| # separators so curl/grep never see a backslash path under Git Bash/MSYS. | ||
| TEMP_DIR="${AGENT_TEMPDIRECTORY:-.}" | ||
| LABELS_FILE="${TEMP_DIR//\\//}/native-auth-pr-labels.json" | ||
|
|
||
| if [ -z "${PR_NUMBER:-}" ]; then | ||
| echo "No PR number available (not a PR build). Skipping Native Auth E2E tests." | ||
| echo "##vso[task.setvariable variable=SKIP_NATIVE_AUTH_E2E]true" | ||
| exit 0 | ||
| fi | ||
|
|
||
| LABELS_URL="https://api.github.com/repos/${REPO}/issues/${PR_NUMBER}/labels" | ||
| echo "Fetching PR labels from ${LABELS_URL}" | ||
| # Capture the body and the HTTP status separately, so that a failed API call is | ||
| # distinguishable from a successful call that simply returned no matching label. | ||
| HTTP_STATUS=$(curl -sS -o "$LABELS_FILE" -w "%{http_code}" "$LABELS_URL" || echo "000") | ||
|
|
||
| if [ "$HTTP_STATUS" != "200" ]; then | ||
| echo "GitHub labels API returned HTTP ${HTTP_STATUS}. Skipping Native Auth E2E tests as a preventive measure (fail-safe to skipping, never to blocking)." | ||
| echo "##vso[task.setvariable variable=SKIP_NATIVE_AUTH_E2E]true" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # jq is not reliably available on the windows-2022 pool, so match on the raw JSON. | ||
| if grep -q "\"name\"[[:space:]]*:[[:space:]]*\"${SKIP_LABEL}\"" "$LABELS_FILE"; then | ||
| echo "Label '${SKIP_LABEL}' is present on PR #${PR_NUMBER}. Skipping Native Auth E2E tests." | ||
| echo "##vso[task.setvariable variable=SKIP_NATIVE_AUTH_E2E]true" | ||
| else | ||
| echo "Label '${SKIP_LABEL}' is not present on PR #${PR_NUMBER}. Running Native Auth E2E tests." | ||
| echo "##vso[task.setvariable variable=SKIP_NATIVE_AUTH_E2E]false" | ||
| fi | ||
| displayName: Check for skip-native-auth-e2e-tests label | ||
| env: | ||
| PR_NUMBER: $(System.PullRequest.PullRequestNumber) | ||
| REPO: $(Build.Repository.Name) | ||
| - task: Gradle@2 | ||
| displayName: Run Native Auth E2E Tests | ||
| # Mirrors iOS (microsoft-authentication-library-for-objc) pr-validation.yml, which guards its | ||
| # native auth E2E jobs with the same opt-out label and a 30 minute timeout. | ||
| condition: and(succeeded(), ne(variables['SKIP_NATIVE_AUTH_E2E'], 'true')) | ||
| timeoutInMinutes: 30 | ||
| inputs: | ||
| tasks: msal:testLocalDebugUnitTest -Plabtest -ProbolectricSdkVersion=${{variables.robolectricSdkVersion}} -PnativeAuthConfigString=$(NATIVE_AUTH_CONFIG_STRING) --tests com.microsoft.identity.client.e2e.tests.network.nativeauth.* | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we gating every PR on these now? They hit mail.tm + a live tenant, and these are the same tests that were iGnOrE (@ignore)'d for OTP flakiness. I feel like unrelated PRs will start failing validation whenever mail.tm has a hiccup or the tenant throttles OTPs. Could these run on a nightly/scheduled pipeline instead of the per-PR gate?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point — a mail.tm hiccup shouldn't fail a PR that never touched Native Auth. I went with an escape hatch rather than nightly: a Keeping them on the gate is what makes a regression attributable to the PR that caused it. The label gives us the unblock without giving up that attribution. Let me know if you think that's a reasonable solution |
||
| javaHomeSelection: $(BuildParameters.javaHomeSelection) | ||
| jdkVersion: 1.17 | ||
| publishJUnitResults: true | ||
| testResultsFiles: '**/build/test-results/testLocalDebugUnitTest/TEST-*.xml' | ||
| testRunTitle: Native Auth E2E Tests (PR Branch) | ||
| env: | ||
| EMAIL_PROVIDER_PASSWORD: $(EMAIL_PROVIDER_PASSWORD) | ||
|
djanardhan98 marked this conversation as resolved.
|
||
| - publish: $(Build.SourcesDirectory)/msal/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml | ||
| artifact: jacocoReport | ||
| displayName: 'Publish JaCoCo Report Artifact (PR Branch)' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // All rights reserved. | ||
| // | ||
| // This code is licensed under the MIT License. | ||
| // | ||
| // Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| // of this software and associated documentation files(the "Software"), to deal | ||
| // in the Software without restriction, including without limitation the rights | ||
| // to use, copy, modify, merge, publish, distribute, sublicense, and / or sell | ||
| // copies of the Software, and to permit persons to whom the Software is | ||
| // furnished to do so, subject to the following conditions: | ||
| // | ||
| // The above copyright notice and this permission notice shall be included in | ||
| // all copies or substantial portions of the Software. | ||
| // | ||
| // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
| package com.microsoft.identity.client.e2e.tests.network.nativeauth | ||
|
|
||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertSame | ||
| import org.junit.Assert.assertThrows | ||
| import org.junit.Test | ||
|
|
||
| class NativeAuthPublicClientApplicationAbstractTestTest : | ||
| NativeAuthPublicClientApplicationAbstractTest() { | ||
|
|
||
| @Test | ||
| fun retryOperationDoesNotRetryNonThrottleExceptions() { | ||
| val expected = IllegalStateException("Non-throttle failure") | ||
| var attempts = 0 | ||
|
|
||
| val actual = assertThrows(IllegalStateException::class.java) { | ||
| retryOperation(maxRetries = 1) { | ||
| attempts++ | ||
| throw expected | ||
|
Comment on lines
+38
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IllegalStateException isn't an AssertionError, so it never reaches the catch (e: AssertionError) in retryOperation. it just falls straight through here, so this is only really checking that a stray RuntimeException isn't swallowed, not the throttle gating. the branch I'd want pinned is a non-701014 AssertionError getting rethrown with no retry (the isThrottleError false -> throw e path). can we add that case? attempts should still be 1 and the original error should come back out. |
||
| } | ||
| } | ||
|
|
||
| assertSame(expected, actual) | ||
| assertEquals(1, attempts) | ||
| } | ||
| } | ||
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.
This label check hits api.github.com unauthenticated. Unauth github API is 60/hr per IP, and on the shared windows-2022 pool that egress IP is shared across a ton of jobs, so on a busy day this'll come back 403 rate-limited. The fail-safe right below then treats any non-200 as skip=true, so the native auth e2e tests we're re-enabling would just silently stop running with a green pipeline and nobody notices. kinda defeats the point of making them a PR gate.