Feat/multiple user support in e2e tests #1182
Conversation
|
@JohnAllenTech is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve significant updates to the environment variable management for end-to-end (E2E) testing across multiple files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (8)
e2e/my-posts.spec.ts (2)
4-7: Implement tests for unauthenticated users.The test suite for unauthenticated users is currently empty. Consider adding tests for:
- Redirect behavior when accessing my-posts page while not logged in
- Error messages or notifications shown to unauthenticated users
Would you like me to help generate these test cases?
13-15: Implement tests for authenticated users.The test suite for authenticated users is currently empty. Consider adding tests for:
- Viewing own posts
- Empty state when user has no posts
- Pagination if applicable
- Post management actions (if available)
Would you like me to help generate these test cases? I can provide examples that align with the multiple user support feature.
e2e/settings.spec.ts (1)
1-15: Consider adding test coverage for user two interactions.Given that this PR introduces support for multiple users, consider adding a third test suite that verifies interactions between user one and user two (e.g., settings visibility between users, permission boundaries).
Would you like me to help design test cases for multi-user interaction scenarios?
sample.env (1)
9-12: Consider adding comments to document the E2E test users.To improve maintainability, consider adding comments explaining the purpose of each E2E user and their intended use in tests.
+# E2E Test User One - Primary user for basic authentication tests E2E_USER_ONE_ID=8e3179ce-f32b-4d0a-ba3b-234d66b836ad +# E2E Test User Two - Secondary user for testing user interactions E2E_USER_TWO_ID=a15a104a-0e34-4101-8800-ed25c9231345 +# Session IDs for E2E test users (must be valid UUIDs) E2E_USER_ONE_SESSION_ID=df8a11f2-f20a-43d6-80a0-a213f1efedc1 E2E_USER_TWO_SESSION_ID=10134766-bc6c-4b52-83d7-46ec0a4cb95de2e/home.spec.ts (1)
Line range hint
9-41: Consider improving test maintainability.The test implementation is correct, but could benefit from some improvements:
- Extract the base URL to a shared configuration:
// e2e/config.ts export const BASE_URL = 'http://localhost:3000'; // Usage in test import { BASE_URL } from './config'; await page.goto(BASE_URL);
- Make test descriptions more specific:
-test("Homepage view", async ({ page, isMobile }) => { +test("should show/hide popular topics based on viewport", async ({ page, isMobile }) => {drizzle/seed.ts (2)
Line range hint
127-153: Consider adding input validation and type safety.The function implementation is solid, but could benefit from:
- Input validation for email format
- UUID validation for id
- Name length constraints
Consider adding validation:
const seedE2EUser = async (email: string, id: string, name: string) => { + if (!email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { + throw new Error('Invalid email format'); + } + if (!id.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i)) { + throw new Error('Invalid UUID format'); + } + if (name.length < 2 || name.length > 100) { + throw new Error('Name must be between 2 and 100 characters'); + } const [existingE2EUser] = await db
Line range hint
155-180: Improve error handling and session management.Several improvements could enhance the robustness of this function:
- The catch block only logs errors without proper error propagation
- Session token format is not validated
- Session expiry duration is hardcoded
Consider these improvements:
const seedE2EUserSession = async (userId: string, sessionToken: string) => { + if (!sessionToken.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i)) { + throw new Error('Invalid session token format'); + } + const SESSION_EXPIRY_MONTHS = 6; const [existingE2EUserSession] = await db .selectDistinct() .from(session) .where(eq(session.sessionToken, sessionToken)); if (existingE2EUserSession) { console.log("E2E Test session already exists. Skipping creation"); return existingE2EUserSession; } try { const currentDate = new Date(); return await db .insert(session) .values({ userId, sessionToken, - // Set session to expire in 6 months. - expires: new Date(currentDate.setMonth(currentDate.getMonth() + 6)), + expires: new Date(currentDate.setMonth(currentDate.getMonth() + SESSION_EXPIRY_MONTHS)), }) .returning(); } catch (err) { - console.log(err); + console.error('Failed to create E2E test session:', err); + throw err; } };README.md (1)
144-169: Consider adding example values for environment variables.To improve developer experience, consider adding example values for these environment variables, similar to how other variables are documented in the README.
Would you like me to provide example values that could be added to the documentation?
🧰 Tools
🪛 LanguageTool
[misspelling] ~147-~147: Did you mean “there”?
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~148-~148: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[misspelling] ~154-~154: Did you mean “there”?
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~155-~155: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[misspelling] ~161-~161: Did you mean “there”?
Context: ...sting . This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~162-~162: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_ID ### E2E_USER_TWO_ID...(DIFFERENT_TO)
[misspelling] ~168-~168: Did you mean “there”?
Context: ...sting . This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~169-~169: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_ID For more information...(DIFFERENT_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
.github/workflows/e2e-tests.ymlis excluded by!**/*.ymlplaywright/.auth/browser.jsonis excluded by!**/*.json
📒 Files selected for processing (13)
- README.md (2 hunks)
- drizzle/seed.ts (5 hunks)
- e2e/articles.spec.ts (3 hunks)
- e2e/auth.setup.ts (0 hunks)
- e2e/home.spec.ts (1 hunks)
- e2e/login.spec.ts (2 hunks)
- e2e/my-posts.spec.ts (1 hunks)
- e2e/settings.spec.ts (1 hunks)
- e2e/teardown.ts (1 hunks)
- e2e/utils/index.ts (1 hunks)
- e2e/utils/utils.ts (1 hunks)
- playwright.config.ts (0 hunks)
- sample.env (1 hunks)
💤 Files with no reviewable changes (2)
- e2e/auth.setup.ts
- playwright.config.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/utils/index.ts
🧰 Additional context used
📓 Learnings (2)
e2e/articles.spec.ts (1)
Learnt from: JohnAllenTech PR: codu-code/codu#1158 File: e2e/articles.spec.ts:208-224 Timestamp: 2024-10-20T01:10:48.663Z Learning: In `e2e/articles.spec.ts`, tests within the `Authenticated Articles Page` describe block assume the user is already authenticated, so explicit authentication steps are not necessary within individual tests.e2e/my-posts.spec.ts (1)
Learnt from: JohnAllenTech PR: codu-code/codu#1158 File: e2e/articles.spec.ts:208-224 Timestamp: 2024-10-20T01:10:48.663Z Learning: In `e2e/articles.spec.ts`, tests within the `Authenticated Articles Page` describe block assume the user is already authenticated, so explicit authentication steps are not necessary within individual tests.
🪛 LanguageTool
README.md
[misspelling] ~147-~147: Did you mean “there”?
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~148-~148: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[misspelling] ~154-~154: Did you mean “there”?
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~155-~155: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[misspelling] ~161-~161: Did you mean “there”?
Context: ...sting . This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~162-~162: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_ID ### E2E_USER_TWO_ID...(DIFFERENT_TO)
[misspelling] ~168-~168: Did you mean “there”?
Context: ...sting . This is currently hardcoded and their is no reason to change this value. **No...(THEIR_IS)
[grammar] ~169-~169: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_ID For more information...(DIFFERENT_TO)
[uncategorized] ~193-~193: Loose punctuation mark.
Context: ...for testing. -E2E_USER_ONE_SESSION_ID: The session id that the user will use t...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (14)
e2e/my-posts.spec.ts (3)
2-2: LGTM!The import statement is clear and follows the established pattern.
10-12: LGTM!The authentication setup using
loggedInAsUserOneis correct and follows the established pattern.
1-15: Verify consistency with other E2E test files.Let's ensure this file follows the same patterns as other E2E test files and that the utility function is used consistently.
✅ Verification successful
The test file follows consistent patterns with other E2E tests
The verification shows that:
- All E2E test files consistently use the
loggedInAsUserOneutility from./utils- The test structure with separate "Authenticated" and "Unauthenticated" describe blocks is consistent across files
- The authentication setup is properly handled in
beforeEachhooks- No direct session cookie manipulation is found in test files, everything goes through the utility functions
The implementation matches the established patterns in the codebase and uses the authentication utilities correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in E2E test patterns and utility usage # Test 1: Check the pattern of beforeEach hooks in other E2E tests echo "Checking beforeEach patterns in E2E tests:" rg -A 3 "beforeEach.*loggedInAsUserOne" "e2e/*.spec.ts" # Test 2: Check for any remaining direct authentication code that should use the utility echo -e "\nChecking for potential direct auth code that should use the utility:" rg -A 5 "setCookie.*session" "e2e/*.spec.ts"Length of output: 460
Script:
#!/bin/bash # Let's first find the correct path for E2E tests echo "Finding E2E test files:" fd ".spec.ts$" # Then analyze the patterns echo -e "\nChecking beforeEach patterns in E2E tests:" fd ".spec.ts$" --exec rg -A 3 "beforeEach.*loggedInAsUserOne" {} echo -e "\nChecking for potential direct auth code that should use the utility:" fd ".spec.ts$" --exec rg -A 5 "setCookie.*session" {} # Check the utility function implementation echo -e "\nChecking loggedInAsUserOne utility implementation:" fd "utils.ts" --exec cat {}Length of output: 3388
Script:
#!/bin/bash # Let's check the authentication patterns in all E2E test files echo "Checking authentication patterns in all E2E tests:" fd ".spec.ts$" --exec cat {} | grep -A 10 "test.describe" # Let's also check if there are any other auth-related utilities being used echo -e "\nChecking for other auth-related imports and usage:" fd ".spec.ts$" --exec rg "import.*loggedIn|import.*auth" {}Length of output: 4757
e2e/settings.spec.ts (2)
1-2: LGTM: Clean import of the new authentication utility.The import of
loggedInAsUserOnealigns with the PR's objective of supporting multiple users in E2E tests.
4-7: Implementation needed: Add unauthenticated user tests.The test suite is currently empty with only a placeholder comment. Consider implementing tests for scenarios like:
- Redirecting unauthenticated users to login
- Displaying appropriate error messages
- Protecting sensitive settings endpoints
Would you like me to help generate the test cases for unauthenticated user scenarios?
sample.env (1)
9-12: LGTM! Good implementation of multiple user support.The implementation uses proper UUID format for both users and their sessions, with unique values preventing any potential conflicts.
e2e/home.spec.ts (1)
2-7: LGTM! Clean authentication setup using the new utility.The introduction of
loggedInAsUserOneutility and its usage in thebeforeEachhook provides a consistent way to handle authentication across tests.Let's verify the utility implementation and its usage across other test files:
✅ Verification successful
Authentication setup is well-implemented and consistently used
The verification shows that:
- The utility is properly implemented using session tokens and cookie management
- It's consistently used across multiple test files (home, articles, login, my-posts, settings)
- There's a companion utility
loggedInAsUserTwo, aligning with the PR's goal of supporting multiple users- The implementation includes proper error handling and assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of loggedInAsUserOne utility # Test 1: Check the utility implementation echo "Checking utility implementation:" rg -A 5 "export const loggedInAsUserOne" # Test 2: Check usage across test files echo "\nChecking usage across test files:" rg "loggedInAsUserOne" "e2e/*.spec.ts"Length of output: 580
Script:
#!/bin/bash # Let's try with fd to find and search through spec files # Find all spec files echo "Finding spec files:" fd ".spec.ts$" "e2e" # Search for loggedInAsUserOne usage echo -e "\nChecking loggedInAsUserOne usage:" fd ".spec.ts$" "e2e" --exec rg "loggedInAsUserOne" {} # Check the complete utility implementation echo -e "\nComplete utility implementation:" rg -A 15 "export const loggedInAsUserOne" "e2e/utils/utils.ts" # Check for other similar login utilities echo -e "\nChecking for other login utilities:" rg "export const" "e2e/utils/utils.ts" | grep -i "login\|user"Length of output: 1564
e2e/utils/utils.ts (2)
1-1: LGTM: Imports are clean and appropriateThe imports are minimal and correctly scoped to the requirements.
1-51: Verify environment variable documentationThe functions rely on environment variables that need to be properly configured for E2E tests to work.
Let's check if these environment variables are documented:
✅ Verification successful
Environment variables are well documented
The environment variables
E2E_USER_ONE_SESSION_IDandE2E_USER_TWO_SESSION_IDare properly documented in:
- README.md with clear explanations and usage instructions
- sample.env with example values provided
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if environment variables are documented in README or env example files echo "Searching for environment variable documentation..." rg -l "E2E_USER_(ONE|TWO)_SESSION_ID" --type md rg -l "E2E_USER_(ONE|TWO)_SESSION_ID" --type-add envfile:*.env* --type envfileLength of output: 266
Script:
#!/bin/bash # Check the actual documentation content echo "=== README.md content ===" rg -A 5 -B 5 "E2E_USER_(ONE|TWO)_SESSION_ID" README.md echo -e "\n=== sample.env content ===" rg -A 2 -B 2 "E2E_USER_(ONE|TWO)_SESSION_ID" sample.envLength of output: 1491
e2e/login.spec.ts (2)
3-3: LGTM: Clean import of authentication utilityThe import of
loggedInAsUserOnefollows good practices and maintains clear naming conventions.
35-37: LGTM: Clean authentication setup with verification neededThe beforeEach hook implementation is clean and follows testing best practices.
Let's verify the implementation of the authentication utility:
✅ Verification successful
Authentication utilities are well-implemented and consistently used
The authentication utilities are properly implemented with:
- Robust session handling using environment variables
- Proper cookie management with security considerations (sameSite: Lax)
- Error handling and validation checks
- Consistent usage of
loggedInAsUserOneacross multiple test filesThe implementation follows testing best practices and is being used consistently throughout the e2e test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the authentication utility implementation and usage # Check the utils implementation echo "Checking utils implementation..." rg -p "loggedInAsUserOne" "e2e/utils" # Check for potential user two implementation echo "Checking for user two implementation..." rg -p "loggedInAsUserTwo" "e2e/utils" # Check for other test files using these utilities echo "Checking usage in other test files..." rg -p "loggedInAsUser(One|Two)" "e2e/*.spec.ts"Length of output: 581
Script:
#!/bin/bash # Let's verify the implementation details and usage with more context # Check the complete implementation of both utility functions echo "Checking complete implementation of auth utilities..." rg -p -A 20 "export const loggedInAsUser" e2e/utils/utils.ts # Check all test files for usage (with correct path) echo "Checking usage in test files..." fd ".spec.ts" "e2e" --exec rg -p "loggedInAsUser(One|Two)"Length of output: 2017
drizzle/seed.ts (1)
13-25: Clarify the temporary status of environment variables.The comment "These can be removed in a follow on PR" needs more context. If these variables are intended to be temporary, consider:
- Adding a TODO with a specific issue/PR reference
- Documenting why they will be removed
- Explaining the migration plan
e2e/articles.spec.ts (2)
3-4: LGTM! Well-structured authentication setup.The centralized authentication logic in the beforeEach hook and the separation of login utilities align well with the multiple user support objective. This approach maintains consistency with the existing test structure where "Authenticated Articles Page" tests assume authentication.
Also applies to: 133-135
254-254: Verify user name consistency across the codebase.The change from "E2E Test User" to "E2E Test User One" aligns with the multiple user support. Let's verify the consistency of this naming across the codebase.
✅ Verification successful
The change is consistent with the new multiple user support
The codebase shows consistent usage of "E2E Test User One" and "E2E Test User Two" in both the seed file and test expectations. The change aligns with the new environment variables and user definitions in
drizzle/seed.tswhich clearly defines two distinct E2E test users.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent user naming across the codebase # Expected: Only "E2E Test User One" should be present, not the old "E2E Test User" echo "Checking for old user name..." rg "E2E Test User[^O]" -g '!*.md' echo "Checking for new user name usage..." rg "E2E Test User One" echo "Checking seed file for user definitions..." rg "E2E_USER" drizzle/seed.tsLength of output: 1605
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
158-169: Enhance documentation with multi-user testing context.Consider adding a brief explanation about why we have two E2E users. This would help developers understand the testing scenarios these users enable (e.g., testing user interactions, comments, etc.).
Add context like this:
### E2E_USER_ONE_ID This is the userId of one of our E2E users and is used for testing. This is currently hardcoded and there is no reason to change this value. +These two E2E users (ONE and TWO) enable testing scenarios that require user interaction, +such as one user commenting on another user's post. **Note: This value must be different from E2E_USER_TWO_ID**
Line range hint
192-193: Consider reorganizing E2E testing documentation.The E2E testing documentation is currently split between the environment variables section and the "End-to-End Testing with Playwright" section. Consider consolidating all E2E testing-related documentation under a single section for better readability and maintenance.
Move all E2E-related environment variables under the "End-to-End Testing with Playwright" section and organize them logically based on their usage in the testing flow.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~147-~147: “their” seems less likely than “there”.
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(AI_HYDRA_LEO_CP_THEIR_THERE)
[grammar] ~148-~148: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[uncategorized] ~154-~154: “their” seems less likely than “there”.
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(AI_HYDRA_LEO_CP_THEIR_THERE)
[grammar] ~155-~155: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~147-~147: “their” seems less likely than “there”.
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(AI_HYDRA_LEO_CP_THEIR_THERE)
[grammar] ~148-~148: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_TWO_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
[uncategorized] ~154-~154: “their” seems less likely than “there”.
Context: ...ession. This is currently hardcoded and their is no reason to change this value. **No...(AI_HYDRA_LEO_CP_THEIR_THERE)
[grammar] ~155-~155: In American English, the usual prepositions after ‘different’ are “from” or “than”.
Context: ...e. Note: This value must be different to E2E_USER_ONE_SESSION_ID ### E2E_USE...(DIFFERENT_TO)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
JohnAllenTech
temporarily deployed
to
production
GitHub Actions
Inactive
|
This ones good to go @NiallJoeMaher |
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
|
Uh oh! @JohnAllenTech, the image you shared is missing helpful alt text. Check #1182 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
✨ Codu Pull Request 💻
Will need to be merged after #1181
Pull Request details
Follow on from #1181. Now creating two E2E users and supporting using them in our E2E test suite.
This unlocks the ability to test interaction between two users in an E2E Test. ie one user comments on another users comment
Any Breaking changes
You can do this by running
Associated Screenshots
All tests passing locally