Refactor oauth system tests to run in separate workers#1664
Open
Refactor oauth system tests to run in separate workers#1664
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPer-spec OAuth tests now run with their own redirect server and dedicated test users via Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant Redirect as Redirect Server
participant Browser as Browser / Playwright
participant AuthServer as Authorization Server
TestRunner->>Redirect: setupRedirectServer()
TestRunner->>Browser: launch context (storageState from getOAuthTestUser)
Browser->>AuthServer: navigate to /authorize
AuthServer->>Browser: show login page
Browser->>AuthServer: submit credentials
alt Research consent visible pre-login
Browser->>AuthServer: accept consent (ensureRedirectServer + submit)
else Research consent appears post-login
Browser->>AuthServer: accept consent (race handling + submit)
end
AuthServer->>Redirect: redirect to callback URL
Redirect->>TestRunner: capture callback/code
TestRunner->>Redirect: teardownRedirectServer()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
system-tests/src/tests/oauth/discovery.spec.ts (1)
14-19: Consider removing redirect server setup for discovery tests.These tests only make
fetch()requests to the well-known and JWKS endpoints—no OAuth redirect flows occur. The redirect server setup adds unnecessary overhead here.That said, if maintaining consistency across all OAuth test files is preferred for simplicity, keeping it is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/oauth/discovery.spec.ts` around lines 14 - 19, The tests in discovery.spec.ts don't use OAuth redirect flows, so remove the unnecessary redirect server setup/teardown by deleting the test.beforeAll(() => setupRedirectServer()) and test.afterAll(() => teardownRedirectServer()) calls; if doing so leaves unused imports or variables related to setupRedirectServer or teardownRedirectServer, remove those imports/usages as well to avoid lint errors and keep the test lightweight and focused on fetch() calls to the well-known and JWKS endpoints.system-tests/src/tests/oauth/revocation.spec.ts (1)
32-108: Consider consolidating duplicate helper functions.
getAccessTokenandgetRefreshTokenare nearly identical—the only difference is the requested scopes. The login/consent handling logic is duplicated across both.♻️ Suggested refactor
- // Helper to get a valid access token using PKCE - async function getAccessToken(page: Page): Promise<string> { + // Helper to complete an OAuth flow and get tokens + async function completeOAuthFlow( + page: Page, + scopes: string[], + ): Promise<{ access_token: string; refresh_token?: string }> { const codeVerifier = generateCodeVerifier() const codeChallenge = generateCodeChallenge(codeVerifier) - const { url, state, scopes } = await oauthUrl(["openid", "email"], { + const { url, state, scopes: requestedScopes } = await oauthUrl(scopes, { codeChallenge, codeChallengeMethod: "S256", }) await page.goto(url) // Handle login - user might already be logged in try { await page.waitForURL(/\/login\?return_to=.*/, { timeout: 2000 }) - // User needs to login await performLogin(page, REVOCATION_EMAIL, REVOCATION_PASSWORD) } catch { - // User is already logged in, skip login page - // We might already be on consent page or callback, so continue + // Already logged in } - // Handle both cases: consent page may appear or may be skipped if already granted - // Wait for navigation after login - we'll either go to consent page or directly to callback try { - // Try to wait for consent page first (with short timeout) await page.waitForURL(/\/oauth_authorize_scopes/, { timeout: 2000 }) - // If we get here, we're on consent page - const consent = new ConsentPage(page, scopes) + const consent = new ConsentPage(page, requestedScopes) await consent.expectVisible(new RegExp(`${APP_DISPLAY_NAME}|${TEST_CLIENT_ID}`)) await consent.approve() } catch { - // Consent page didn't appear (already granted), wait for callback instead await page.waitForURL(/callback/, { timeout: 10000 }) } const code = await assertAndExtractCodeFromCallbackUrl(page, state) const tok = await exchangeCodeForToken(code, { kind: "bearer" }, codeVerifier) - return tok.access_token + return tok } - // Helper to get a valid refresh token using PKCE - async function getRefreshToken(page: Page): Promise<string> { - const codeVerifier = generateCodeVerifier() - const codeChallenge = generateCodeChallenge(codeVerifier) - const { url, state, scopes } = await oauthUrl(["openid", "offline_access"], { - codeChallenge, - codeChallengeMethod: "S256", - }) - await page.goto(url) - - // Handle login - user might already be logged in - try { - await page.waitForURL(/\/login\?return_to=.*/, { timeout: 2000 }) - // User needs to login - await performLogin(page, REVOCATION_EMAIL, REVOCATION_PASSWORD) - } catch { - // User is already logged in, skip login page - // We might already be on consent page or callback, so continue - } - - // Handle both cases: consent page may appear or may be skipped if already granted - // Wait for navigation after login - we'll either go to consent page or directly to callback - try { - // Try to wait for consent page first (with short timeout) - await page.waitForURL(/\/oauth_authorize_scopes/, { timeout: 2000 }) - // If we get here, we're on consent page - const consent = new ConsentPage(page, scopes) - await consent.expectVisible(new RegExp(`${APP_DISPLAY_NAME}|${TEST_CLIENT_ID}`)) - await consent.approve() - } catch { - // Consent page didn't appear (already granted), wait for callback instead - await page.waitForURL(/callback/, { timeout: 10000 }) - } - - const code = await assertAndExtractCodeFromCallbackUrl(page, state) - const tok = await exchangeCodeForToken(code, { kind: "bearer" }, codeVerifier) - expect(tok.refresh_token).toBeTruthy() - return tok.refresh_token! + async function getAccessToken(page: Page): Promise<string> { + const tok = await completeOAuthFlow(page, ["openid", "email"]) + return tok.access_token + } + + async function getRefreshToken(page: Page): Promise<string> { + const tok = await completeOAuthFlow(page, ["openid", "offline_access"]) + expect(tok.refresh_token).toBeTruthy() + return tok.refresh_token! }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/tests/oauth/revocation.spec.ts` around lines 32 - 108, Extract the duplicated flow in getAccessToken and getRefreshToken into a single helper (e.g., obtainTokenWithPkce) that accepts the requested scopes and a flag to assert presence of refresh_token; move the shared steps (generateCodeVerifier/generateCodeChallenge, call oauthUrl, page.goto, login handling via performLogin, consent handling via ConsentPage, assertAndExtractCodeFromCallbackUrl, and exchangeCodeForToken) into that helper and return the token object, then replace getAccessToken with a thin wrapper that calls obtainTokenWithPkce(["openid","email"]) and returns access_token, and replace getRefreshToken with a thin wrapper that calls obtainTokenWithPkce(["openid","offline_access"]) and asserts tok.refresh_token before returning it; reference symbols: getAccessToken, getRefreshToken, oauthUrl, generateCodeVerifier, generateCodeChallenge, performLogin, ConsentPage, assertAndExtractCodeFromCallbackUrl, exchangeCodeForToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@system-tests/src/utils/oauth/constants.ts`:
- Around line 35-36: OAUTH_SPEC_USERS is currently annotated with Record<string,
OAuthTestUser>, which widens keys to string/number and loses compile-time key
validation; remove the explicit type annotation and use the TypeScript satisfies
operator instead (replace "const OAUTH_SPEC_USERS: Record<string, OAuthTestUser>
= { ... }" with "const OAUTH_SPEC_USERS = { ... } satisfies Record<string,
OAuthTestUser>;") so the object keeps literal keys for keyof typeof
OAUTH_SPEC_USERS while still ensuring each value matches OAuthTestUser; then
rebuild/tsc to verify keyof now yields the literal union used by the runtime
check around line 112.
In `@system-tests/src/utils/oauth/loginHelpers.ts`:
- Around line 13-16: The click on the Save button should be synchronized with
waitForSuccessNotification to ensure the backend mutation completed; replace the
bare await page.getByRole("button", { name: "Save" }).click() calls (both the
one after page.getByLabel(/I want to participate in the educational
research/).click() and the similar Save click at the later occurrence) with a
pattern that calls waitForSuccessNotification(...) around the click so the test
waits for the success toast before waiting for the URL or returning; use the
existing waitForSuccessNotification helper and keep the surrounding steps (the
consent checkbox click and the subsequent page.waitForURL) intact.
---
Nitpick comments:
In `@system-tests/src/tests/oauth/discovery.spec.ts`:
- Around line 14-19: The tests in discovery.spec.ts don't use OAuth redirect
flows, so remove the unnecessary redirect server setup/teardown by deleting the
test.beforeAll(() => setupRedirectServer()) and test.afterAll(() =>
teardownRedirectServer()) calls; if doing so leaves unused imports or variables
related to setupRedirectServer or teardownRedirectServer, remove those
imports/usages as well to avoid lint errors and keep the test lightweight and
focused on fetch() calls to the well-known and JWKS endpoints.
In `@system-tests/src/tests/oauth/revocation.spec.ts`:
- Around line 32-108: Extract the duplicated flow in getAccessToken and
getRefreshToken into a single helper (e.g., obtainTokenWithPkce) that accepts
the requested scopes and a flag to assert presence of refresh_token; move the
shared steps (generateCodeVerifier/generateCodeChallenge, call oauthUrl,
page.goto, login handling via performLogin, consent handling via ConsentPage,
assertAndExtractCodeFromCallbackUrl, and exchangeCodeForToken) into that helper
and return the token object, then replace getAccessToken with a thin wrapper
that calls obtainTokenWithPkce(["openid","email"]) and returns access_token, and
replace getRefreshToken with a thin wrapper that calls
obtainTokenWithPkce(["openid","offline_access"]) and asserts tok.refresh_token
before returning it; reference symbols: getAccessToken, getRefreshToken,
oauthUrl, generateCodeVerifier, generateCodeChallenge, performLogin,
ConsentPage, assertAndExtractCodeFromCallbackUrl, exchangeCodeForToken.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
system-tests/src/tests/oauth/authorize/boundary.spec.tssystem-tests/src/tests/oauth/authorize/code-issuance.spec.tssystem-tests/src/tests/oauth/authorize/parameter-validation.spec.tssystem-tests/src/tests/oauth/authorize/pkce.spec.tssystem-tests/src/tests/oauth/authorize/user-auth.spec.tssystem-tests/src/tests/oauth/discovery.spec.tssystem-tests/src/tests/oauth/flows.spec.tssystem-tests/src/tests/oauth/index.spec.tssystem-tests/src/tests/oauth/introspect.spec.tssystem-tests/src/tests/oauth/revocation.spec.tssystem-tests/src/tests/oauth/token/authorization-code.spec.tssystem-tests/src/tests/oauth/token/client-auth.spec.tssystem-tests/src/tests/oauth/token/issuance.spec.tssystem-tests/src/tests/oauth/token/parameter-validation.spec.tssystem-tests/src/tests/oauth/token/refresh.spec.tssystem-tests/src/tests/oauth/userinfo/bearer.spec.tssystem-tests/src/tests/oauth/userinfo/dpop.spec.tssystem-tests/src/tests/oauth/userinfo/scopes.spec.tssystem-tests/src/utils/oauth/constants.tssystem-tests/src/utils/oauth/loginHelpers.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
system-tests/src/utils/oauth/loginHelpers.ts (1)
17-27: Consider extracting shared consent-submit logic to a helper.The pre-login and post-login consent paths duplicate the same checkbox + Save + URL wait sequence.
Refactor sketch
export async function performLogin(page: Page, email: string, password: string) { await ensureRedirectServer() + const acceptResearchConsentIfVisible = async (): Promise<boolean> => { + const dialog = page.getByTestId("research-consent-dialog") + if (!(await dialog.isVisible())) return false + await page.getByLabel(/I want to participate in the educational research/).click() + await waitForSuccessNotification(page, async () => { + await page.getByRole("button", { name: "Save" }).click() + }) + await page.waitForURL(/\/authorize|\/oauth_authorize_scopes|\/callback/, { + timeout: 10000, + waitUntil: "domcontentloaded", + }) + return true + } + - const consentDialog = page.getByTestId("research-consent-dialog") - if (await consentDialog.isVisible()) { - ... - return - } + if (await acceptResearchConsentIfVisible()) return // login... - if (await page.getByTestId("research-consent-dialog").isVisible()) { - ... - } + await acceptResearchConsentIfVisible() }Also applies to: 47-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-tests/src/utils/oauth/loginHelpers.ts` around lines 17 - 27, Extract the duplicated consent submit sequence into a helper (e.g., submitConsentIfVisible) to remove the repeated checkbox + Save + URL wait logic: move the block that checks consentDialog = page.getByTestId("research-consent-dialog"), clicks page.getByLabel(/I want to participate in the educational research/), calls waitForSuccessNotification(page, async () => page.getByRole("button", { name: "Save" }).click()), and awaits page.waitForURL(/\/authorize|\/oauth_authorize_scopes|\/callback/, { timeout: 10000, waitUntil: "domcontentloaded" }) into that helper (accepting the Playwright Page), then replace the original inline sequence in both locations (the block referencing consentDialog and the one at 47-56) with a single await submitConsentIfVisible(page) call so behavior and returned control flow remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@system-tests/src/tests/oauth/authorize/user-auth.spec.ts`:
- Around line 52-55: The catch is swallowing all errors (including performLogin
failures); change the flow so only a wait-for-URL timeout is treated as "already
logged in" while any performLogin error is rethrown: call
page.waitForURL(/\/login\?return_to=.*/, { timeout: 2000 }) in a try, and if it
succeeds call performLogin(page, USER_AUTH_USER.email, USER_AUTH_USER.password);
in the catch inspect the error (e.g. err.name === 'TimeoutError' or equivalent)
and return/continue only for that case, otherwise throw the error so
performLogin failures surface.
In `@system-tests/src/utils/oauth/redirectServer.ts`:
- Around line 97-103: The shared redirect server liveness isn't checked, so
ensureRedirectServer() should call the existing _verifyServerRunning() and, if
it returns false, clear the stale shared state and recreate the server;
specifically, in ensureRedirectServer() invoke _verifyServerRunning(), and when
it indicates the server is not running set _isSharedServer = false (and adjust
_setupCount if needed) then proceed to run the server-creation/setup path (the
same logic used when no shared server exists) so workers get a live server
instead of a dead shared flag.
---
Nitpick comments:
In `@system-tests/src/utils/oauth/loginHelpers.ts`:
- Around line 17-27: Extract the duplicated consent submit sequence into a
helper (e.g., submitConsentIfVisible) to remove the repeated checkbox + Save +
URL wait logic: move the block that checks consentDialog =
page.getByTestId("research-consent-dialog"), clicks page.getByLabel(/I want to
participate in the educational research/), calls
waitForSuccessNotification(page, async () => page.getByRole("button", { name:
"Save" }).click()), and awaits
page.waitForURL(/\/authorize|\/oauth_authorize_scopes|\/callback/, { timeout:
10000, waitUntil: "domcontentloaded" }) into that helper (accepting the
Playwright Page), then replace the original inline sequence in both locations
(the block referencing consentDialog and the one at 47-56) with a single await
submitConsentIfVisible(page) call so behavior and returned control flow remain
identical.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
system-tests/src/tests/oauth/authorize/code-issuance.spec.tssystem-tests/src/tests/oauth/authorize/user-auth.spec.tssystem-tests/src/tests/oauth/introspect.spec.tssystem-tests/src/tests/oauth/revocation.spec.tssystem-tests/src/tests/oauth/token/authorization-code.spec.tssystem-tests/src/tests/oauth/token/client-auth.spec.tssystem-tests/src/tests/oauth/token/issuance.spec.tssystem-tests/src/tests/oauth/token/parameter-validation.spec.tssystem-tests/src/tests/oauth/token/refresh.spec.tssystem-tests/src/tests/oauth/userinfo/bearer.spec.tssystem-tests/src/tests/oauth/userinfo/dpop.spec.tssystem-tests/src/tests/oauth/userinfo/scopes.spec.tssystem-tests/src/utils/oauth/consentPage.tssystem-tests/src/utils/oauth/loginHelpers.tssystem-tests/src/utils/oauth/redirectServer.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- system-tests/src/tests/oauth/token/authorization-code.spec.ts
- system-tests/src/tests/oauth/userinfo/bearer.spec.ts
- system-tests/src/tests/oauth/userinfo/dpop.spec.ts
- system-tests/src/tests/oauth/token/client-auth.spec.ts
- system-tests/src/tests/oauth/token/parameter-validation.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@system-tests/src/tests/oauth/authorize/user-auth.spec.ts`:
- Around line 50-56: Clear any pre-existing OAuth grants for the test user and
client before asserting the consent/scopes page so the test is deterministic:
add a call to the existing test cleanup or API helper (e.g., a
revokeGrantsForUser or clearUserGrants helper) using USER_AUTH_USER.email (or
id) and TEST_CLIENT_ID immediately after performLogin (or just before awaiting
/oauth_authorize_scopes) so that ConsentPage.expectVisible and consent.approve
always run in a first-consent state.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
system-tests/src/tests/oauth/authorize/user-auth.spec.tssystem-tests/src/utils/oauth/loginHelpers.tssystem-tests/src/utils/oauth/redirectServer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit