test(e2e): Add an e2e test for Handshake (prod) #6072
Conversation
to remove pauses, debug code
and gherkin style comments
…patibility for signing in
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
jfoshee
changed the title
| test.describe.configure({ mode: 'serial' }); | ||
|
|
||
| test.describe('with Production instance', () => { | ||
| // TODO: change host name |
There was a problem hiding this comment.
Do we need to address the TODO here?
Probably a low priority for the time being, but leaving some high-level instructions here for when we get to this:
- create a new
.clerk.apphost domain - create and configure a new Clerk prod app
- update the local certificates used by the tests to include the new domain in addition to the existing ones
- finally, update the tests
There was a problem hiding this comment.
OK sounds like a task for later. I added that info to an appendix in the integration README.
📝 WalkthroughWalkthroughThe changes introduce a new Playwright end-to-end test suite for the handshake flow in a production-like environment, targeting a specific app instance with a dedicated environment configuration. Supporting updates include the addition of a new environment ( Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration/tests/handshake/handshake.test.ts (1)
15-16: Address the TODO for production host configuration.Based on past review comments, this TODO is acknowledged as a lower priority task. The current setup works for testing but will need updating when moving to a dedicated production host.
🧹 Nitpick comments (3)
integration/README.md (1)
594-606: Good documentation addition with minor suggestions for clarity.The new "Production Hosts" section effectively documents the DNS and proxy infrastructure for production testing.
Consider these minor improvements for clarity:
-Production instances necessitate the use of DNS hostnames. +Production test instances require DNS hostnames for proper testing. -During a test, a local proxy is established to direct requests from the DNS host to a local server. +During tests, a local proxy routes requests from the DNS hostname to the local development server. -To incorporate a new hostname: +To add a new hostname:integration/tests/handshake/handshake.test.ts (2)
43-43: Consider safer type handling for fake user creation.The type assertion
as FakeUserWithEmailcould be unsafe ifcreateFakeUser({ withEmail: true })doesn't guarantee the return type. Consider using type guards or ensuring the function signature properly types the return value.- fakeUser = u.services.users.createFakeUser({ withEmail: true }) as FakeUserWithEmail; + const tempUser = u.services.users.createFakeUser({ withEmail: true }); + if (!tempUser.email) { + throw new Error('Expected user with email'); + } + fakeUser = tempUser as FakeUserWithEmail;
28-40: Consider extracting proxy server setup to a helper function.The proxy server setup logic is correct, but as noted in the TODO comment, it could be factored out into a reusable helper function for other tests that need similar functionality.
Would you like me to help create a reusable proxy server helper function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
integration/README.md(1 hunks)integration/presets/envs.ts(2 hunks)integration/presets/longRunningApps.ts(1 hunks)integration/testUtils/index.ts(1 hunks)integration/testUtils/usersService.ts(1 hunks)integration/tests/handshake/handshake.test.ts(1 hunks)integration/tests/sessions/prod-app-migration.test.ts(0 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- integration/tests/sessions/prod-app-migration.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.md`: Follow all instructions, patterns and conventions that are defined in these MD files.
**/*.md: Follow all instructions, patterns and conventions that are defined in these MD files.
integration/README.md
🔇 Additional comments (9)
integration/testUtils/usersService.ts (1)
54-54: LGTM! Clean type definition.The
FakeUserWithEmailtype appropriately extendsFakeUserwhile ensuring theintegration/presets/envs.ts (2)
197-197: LGTM! Consistent export pattern.The export follows the established pattern for making environment configurations available.
45-50: Verify the emptyCLERK_JS_URLconfiguration.The environment configuration follows the established pattern, but setting
CLERK_JS_URLto an empty string differs from other environments. Please confirm this is intentional for production testing scenarios.#!/bin/bash # Description: Check how CLERK_JS_URL is configured in other environments # Expected: Most environments set CLERK_JS_URL to a specific URL echo "Checking CLERK_JS_URL configurations across environments:" rg "CLERK_JS_URL" integration/presets/envs.ts -A 1 -B 1integration/testUtils/index.ts (1)
9-9: LGTM! Proper type re-export.The import and export of
FakeUserWithEmailfollows the established pattern and makes the type available for use in test files.Also applies to: 12-12
integration/presets/longRunningApps.ts (1)
27-27: LGTM! Consistent long-running app configuration.The new
next.appRouter.sessionsProd1configuration follows the established pattern and correctly uses the new production environment for testing.package.json (1)
42-42: LGTM! Script follows established patterns.The new handshake integration test script correctly follows the pattern of other integration test scripts, with appropriate environment variables and filtering.
integration/tests/handshake/handshake.test.ts (3)
1-12: LGTM! Clean imports and appropriate test structure.The imports are well-organized and the use of serial mode is appropriate for tests that maintain state across multiple test steps.
47-82: LGTM! Comprehensive handshake flow testing.The test logic effectively verifies the handshake flow by:
- Establishing a signed-in state
- Forcing handshake by clearing client UAT cookies
- Verifying proper cookie restoration and absence of temporary cookies
The assertions are thorough and cover the essential aspects of the handshake mechanism.
76-77: Refresh cookie verification is already implemented.The test appropriately checks for refresh cookies as suggested in the past review comments. The assertion ensures that refresh cookies are present after the handshake flow completes.
Description
Add a test of basic handshake flow against a production instance (for reason of no client UAT cookie).
Updated sessionsProd1 environment.
Remove an obsolete E2E test.
Co-author @nikosdouvlis
SDKI-1024
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Documentation
Chores
Tests