chore: moving out user auth out into a util for E2E tests #1181
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 pull request introduces significant changes to the end-to-end test suite for the articles page, focusing on user authentication. It separates tests for authenticated and unauthenticated users, removing the cookie-clearing step for unauthenticated tests and adding a login step for authenticated tests. A new utility function, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
e2e/settings.spec.ts (2)
4-7: Add placeholder comment for test implementationThe comment "Replace with tests for unauthenticated users" should be more descriptive about what needs to be implemented.
- // - // Replace with tests for unauthenticated users + // TODO: Implement tests for unauthenticated user scenarios: + // - Verify redirect to login page + // - Verify unauthorized access messages + // - Verify public content visibility
13-15: Add placeholder comment for authenticated testsSimilar to the unauthenticated section, the comment should be more descriptive about the test scenarios to implement.
- // - // Replace with tests for authenticated users + // TODO: Implement tests for authenticated user scenarios: + // - Verify settings form functionality + // - Verify user profile updates + // - Verify permission-based featurese2e/utils/utils.ts (1)
1-27: Consider a more generic authentication utility.Given that
loggedInAsUserTwowill be added in a future PR, consider refactoring this into a more generic function to avoid duplication:export const loginAsUser = async (page: Page, userNumber: 1 | 2) => { const envVar = `E2E_USER_${userNumber}_SESSION_ID`; // ... rest of the logic using the dynamic environment variable }; // Can then be used as: export const loggedInAsUserOne = (page: Page) => loginAsUser(page, 1); export const loggedInAsUserTwo = (page: Page) => loginAsUser(page, 2);e2e/home.spec.ts (2)
Line range hint
31-46: Consider adding explicit state cleanup for unauthenticated tests.While removing cookie clearing aligns with the refactoring, consider adding a more comprehensive state cleanup to ensure test isolation. This could prevent potential state leakage between tests.
test.describe("Unauthenticated homepage", () => { + test.beforeEach(async ({ page }) => { + // Ensure clean state for unauthenticated tests + await page.context().clearCookies(); + await page.context().clearStorageState(); + }); test("Homepage view", async ({ page }) => {
Line range hint
8-46: Consider enhancing test coverage.While the basic happy path is covered, consider adding tests for:
- Error states (e.g., network issues during auth)
- More comprehensive mobile-specific behaviors
- Navigation paths specific to authenticated/unauthenticated states
Would you like me to help generate additional test cases for these scenarios?
e2e/login.spec.ts (1)
Line range hint
39-45: Consider adding explicit authentication state verificationWhile redirecting to homepage is a good check, consider adding explicit verification of the authenticated state. This would make the test more robust and self-documenting.
test("Sign up page contains sign up links", async ({ page, isMobile }) => { // authenticated users are kicked back to the homepage if they try to go to /get-started await page.goto("http://localhost:3000/get-started"); expect(page.url()).toEqual("http://localhost:3000/"); + // Verify authenticated state + await expect(page.getByTestId("user-profile-dropdown")).toBeVisible(); await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();e2e/articles.spec.ts (1)
Line range hint
183-196: Consider extracting the delay value into a named constant.The 100ms delay value would be more maintainable as a named constant at the top of the file. This makes it easier to adjust if needed and documents its purpose.
+const ARTICLE_LOADING_DELAY_MS = 100; + test("Should load more articles when scrolling to the end of the page", async ({ page, isMobile, }) => { // ... await page.route("**/*", async (route) => { - await new Promise((f) => setTimeout(f, 100)); + await new Promise((f) => setTimeout(f, ARTICLE_LOADING_DELAY_MS)); await route.continue(); }); // ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
playwright/.auth/browser.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
- e2e/articles.spec.ts (2 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/utils/index.ts (1 hunks)
- e2e/utils/utils.ts (1 hunks)
- playwright.config.ts (0 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 (1)
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.
🔇 Additional comments (12)
e2e/my-posts.spec.ts (2)
2-2: LGTM! Clean implementation of the authentication setup.The addition of
loggedInAsUserOneutility and its usage in thebeforeEachhook provides a consistent authentication approach across test suites.Also applies to: 10-12
4-15: Implementation needed: Add test cases for both authenticated and unauthenticated scenarios.Both test suites are currently empty with only TODO comments. Consider implementing:
For unauthenticated suite:
- Redirect to login page
- Access denied messages
- Limited view capabilities
For authenticated suite:
- View own posts
- Create/edit/delete posts
- Pagination/filtering if applicable
Let's check for existing test patterns in other specs:
Would you like me to help draft the test implementations based on existing patterns in the codebase?
✅ Verification successful
Let me gather more specific information about test patterns in the codebase.
The first command will help us understand the test suite structure, and the second will show us authentication-related test patterns that we can follow. This will give us better context for implementing the authenticated and unauthenticated test cases.
Let me gather one more piece of information to understand the my-posts page functionality.
This will help us understand the expected behavior and functionality of the my-posts page to write appropriate test cases.
Based on the gathered information from the codebase, I can now provide a complete response for implementing the test cases.
Add test cases for authenticated and unauthenticated my-posts scenarios
For unauthenticated suite:
- Test redirection to login page when accessing /my-posts
- Verify URL is changed to /get-started
- Ensure login page elements are visible
For authenticated suite:
- Test access to my-posts page after login
- Verify drafts tab functionality (based on ?tab=drafts route parameter)
- Verify scheduled posts tab (based on ?tab=scheduled route parameter)
- Test navigation to create new post
- Ensure "Your Posts" link is visible in nav
Example implementation:
test.describe("Unauthenticated my-posts Page", () => { test("should redirect to login page", async ({ page }) => { await page.goto("http://localhost:3000/my-posts"); await expect(page).toHaveURL("http://localhost:3000/get-started"); await expect(page.getByText("CodúBetaSign in or create")).toBeVisible(); }); }); test.describe("Authenticated my-posts Page", () => { test.beforeEach(async ({ page }) => { await loggedInAsUserOne(page); }); test("should show my posts page", async ({ page }) => { await page.goto("http://localhost:3000/my-posts"); await expect(page.getByRole("link", { name: "Your Posts" })).toBeVisible(); }); test("should handle draft posts tab", async ({ page }) => { await page.goto("http://localhost:3000/my-posts?tab=drafts"); // Add assertions for draft posts view }); test("should handle scheduled posts tab", async ({ page }) => { await page.goto("http://localhost:3000/my-posts?tab=scheduled"); // Add assertions for scheduled posts view }); });🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar test patterns in other spec files rg -l "describe\(\".*Page\"" "e2e/*.spec.ts" | xargs rg -A 10 "test\("Length of output: 19541
Script:
#!/bin/bash # Search for test patterns in e2e tests fd -e ts -p "e2e" | xargs rg -A 5 "test\.describe\(.*\{" # Search for auth-related test patterns fd -e ts -p "e2e" | xargs rg -A 10 "loggedInAs"Length of output: 9370
Script:
#!/bin/bash # Search for routes and components related to my-posts page rg -g '!*.spec.ts' -A 5 "my-posts|MyPosts" # Look for any API endpoints related to my-posts rg -g '*/api/**' "my-posts|user.*posts"Length of output: 3350
e2e/settings.spec.ts (1)
10-12: LGTM: Authentication setup looks goodThe beforeEach hook correctly implements the new authentication pattern using the utility function.
Let's verify the utility function implementation:
✅ Verification successful
Authentication utility implementation and usage verified
The
loggedInAsUserOneutility function is properly implemented ine2e/utils/utils.tsusing session token authentication and is consistently imported and used across all e2e test files (settings, articles, home, login, my-posts). The implementation correctly:
- Validates the presence of the session token environment variable
- Sets up authentication by adding the session cookie with proper security attributes
- Is used in beforeEach hooks for authenticated test scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the loggedInAsUserOne utility implementation # Expected: Should find the utility function definition and its usage pattern # Search for the utility function definition echo "Checking utility function implementation:" ast-grep --pattern 'export async function loggedInAsUserOne($_) { $$$ }' # Search for consistent usage pattern across test files echo "\nChecking usage pattern across test files:" rg -l 'loggedInAsUserOne' 'e2e/*.spec.ts'Length of output: 362
Script:
#!/bin/bash # Let's try a more comprehensive search approach # First, find all e2e test files echo "Finding e2e test files:" fd ".spec.ts$" --type f # Then search for the loggedInAsUserOne function definition echo -e "\nSearching for loggedInAsUserOne function definition:" rg "export.*function loggedInAsUserOne" -A 5 # Look for imports of loggedInAsUserOne echo -e "\nChecking imports of loggedInAsUserOne:" rg "import.*loggedInAsUserOne" # Check all usages of loggedInAsUserOne echo -e "\nChecking all usages of loggedInAsUserOne:" rg "loggedInAsUserOne" -A 2Length of output: 2669
Script:
#!/bin/bash # Let's check the implementation details of the utility function echo "Checking the complete implementation of loggedInAsUserOne:" rg -A 10 "export const loggedInAsUserOne = async \(page: Page\)" e2e/utils/utils.tsLength of output: 563
e2e/utils/utils.ts (2)
1-3: LGTM! Clear and appropriate function signature.The imports and function signature are well-structured for E2E test authentication.
7-15: 🛠️ Refactor suggestionEnhance cookie security and configuration.
Consider these improvements:
- Add
httpOnly: trueto prevent XSS attacks- Make the domain configurable via environment variable
- Consider adding
secure: truefor production environmentsawait page.context().addCookies([ { name: "next-auth.session-token", - value: process.env.E2E_USER_ONE_SESSION_ID as string, + value: sessionId, domain: "localhost", path: "/", sameSite: "Lax", + httpOnly: true, + secure: process.env.NODE_ENV === 'production', }, ]);e2e/home.spec.ts (2)
1-2: LGTM! Good separation of concerns.The import of
loggedInAsUserOnefrom utils aligns with the PR objective of centralizing authentication logic.
5-7: Verify error handling in loggedInAsUserOne utility.While the authentication setup is clean, we should ensure proper error handling exists in the utility function.
✅ Verification successful
Error handling in loggedInAsUserOne is properly implemented
The utility function has proper error handling with a try block and validates the required environment variable. The implementation is secure and follows best practices:
- Validates E2E_USER_ONE_SESSION_ID environment variable
- Uses try-catch block for error handling
- Sets secure cookie attributes (sameSite: "Lax")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of loggedInAsUserOne for error handling ast-grep --pattern 'export async function loggedInAsUserOne($_) { $$$ }'Length of output: 76
Script:
#!/bin/bash # Try finding the loggedInAsUserOne function with ripgrep rg "loggedInAsUserOne" -A 10Length of output: 5424
e2e/login.spec.ts (2)
3-3: LGTM: Clean utility importThe import of
loggedInAsUserOnealigns with the PR objective of moving authentication logic into utilities.
35-37: Verify the implementation of loggedInAsUserOne utilityLet's ensure the authentication utility is properly implemented.
✅ Verification successful
Let me gather more context about the authentication setup and error handling.
Authentication utility implementation looks good with minor improvements needed
The
loggedInAsUserOneutility is properly implemented with:
- Session token management via environment variables
- Cookie setup with appropriate security attributes
- Error handling with custom error messages
- Session verification after setup
However, the error handling could be improved:
- Remove the commented-out console.log
- Add more descriptive error message including the original error details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of loggedInAsUserOne # Check the implementation echo "Checking loggedInAsUserOne implementation..." rg -A 10 "export.*loggedInAsUserOne" ./e2e/utils/Length of output: 732
Script:
#!/bin/bash # Check for error handling and session verification rg -A 5 "next-auth.session-token" ./e2e # Check for any auth-related test utilities and helpers rg -A 5 "auth|session" ./e2e/utils/Length of output: 1364
e2e/articles.spec.ts (3)
3-3: LGTM: Clean utility import for auth.The import of
loggedInAsUserOnealigns with the PR objective of centralizing authentication logic.
133-135: LGTM: Clean authentication setup.The beforeEach hook properly establishes authentication state for all tests in this describe block, which aligns with the existing assumption noted in the retrieved learning.
3-3: Verify the implementation of loggedInAsUserOne utility.Let's ensure the utility function is properly implemented with appropriate error handling and session management.
Also applies to: 133-135
✅ Verification successful
Implementation of loggedInAsUserOne utility looks robust and well-structured
The utility function is properly implemented with:
- Session management using cookies with appropriate security attributes (sameSite: Lax)
- Environment variable validation (E2E_USER_ONE_SESSION_ID)
- Error handling with specific error message for authentication failures
- Clean try-catch block structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of loggedInAsUserOne utility # Check the implementation echo "Checking loggedInAsUserOne implementation..." rg -A 10 "export.*loggedInAsUserOne" ./e2e # Check for error handling echo "Checking error handling..." rg "throw|catch|error" -C 3 ./e2e/utilsLength of output: 1227
| import test from "@playwright/test"; | ||
| import { loggedInAsUserOne } from "./utils"; | ||
|
|
||
| test.describe("Unauthenticated setttings Page", () => { |
There was a problem hiding this comment.
Fix typo in test description
There's a typo in "setttings" (three t's) in the test suite description.
-test.describe("Unauthenticated setttings Page", () => {
+test.describe("Unauthenticated settings Page", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.describe("Unauthenticated setttings Page", () => { | |
| test.describe("Unauthenticated settings Page", () => { |
| import test from "@playwright/test"; | ||
| import { loggedInAsUserOne } from "./utils"; | ||
|
|
||
| test.describe("Unauthenticated setttings Page", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.context().clearCookies(); | ||
| }); | ||
| // | ||
| // Replace with tests for unauthenticated users | ||
| }); | ||
|
|
||
| test.describe("Authenticated settings Page", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await loggedInAsUserOne(page); | ||
| }); | ||
| // | ||
| // Replace with tests for authenticated users | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test organization comments
To improve maintainability and clarity, consider adding JSDoc comments for each test suite to document the test organization and coverage goals.
import test from "@playwright/test";
import { loggedInAsUserOne } from "./utils";
+/**
+ * Test suite for unauthenticated user interactions with the settings page.
+ * Verifies proper handling of unauthorized access and redirects.
+ */
test.describe("Unauthenticated settings Page", () => {
// TODO: Implement tests for unauthenticated scenarios
});
+/**
+ * Test suite for authenticated user interactions with the settings page.
+ * Verifies settings management functionality for logged-in users.
+ */
test.describe("Authenticated settings Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
// TODO: Implement tests for authenticated scenarios
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import test from "@playwright/test"; | |
| import { loggedInAsUserOne } from "./utils"; | |
| test.describe("Unauthenticated setttings Page", () => { | |
| test.beforeEach(async ({ page }) => { | |
| await page.context().clearCookies(); | |
| }); | |
| // | |
| // Replace with tests for unauthenticated users | |
| }); | |
| test.describe("Authenticated settings Page", () => { | |
| test.beforeEach(async ({ page }) => { | |
| await loggedInAsUserOne(page); | |
| }); | |
| // | |
| // Replace with tests for authenticated users | |
| }); | |
| import test from "@playwright/test"; | |
| import { loggedInAsUserOne } from "./utils"; | |
| /** | |
| * Test suite for unauthenticated user interactions with the settings page. | |
| * Verifies proper handling of unauthorized access and redirects. | |
| */ | |
| test.describe("Unauthenticated settings Page", () => { | |
| // | |
| // Replace with tests for unauthenticated users | |
| }); | |
| /** | |
| * Test suite for authenticated user interactions with the settings page. | |
| * Verifies settings management functionality for logged-in users. | |
| */ | |
| test.describe("Authenticated settings Page", () => { | |
| test.beforeEach(async ({ page }) => { | |
| await loggedInAsUserOne(page); | |
| }); | |
| // | |
| // Replace with tests for authenticated users | |
| }); |
e2e/utils/utils.ts
Outdated
| // console.log(err); | ||
|
|
||
| throw Error("Error while authenticating E2E test user one"); | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging.
The error handling could be enhanced:
- Remove commented-out code
- Include the original error details in the thrown error
- Consider adding structured logging for debugging
- // console.log(err);
-
- throw Error("Error while authenticating E2E test user one");
+ throw new Error(
+ `Error while authenticating E2E test user one: ${err instanceof Error ? err.message : String(err)}`
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // console.log(err); | |
| throw Error("Error while authenticating E2E test user one"); | |
| } | |
| throw new Error( | |
| `Error while authenticating E2E test user one: ${err instanceof Error ? err.message : String(err)}` | |
| ); |
| expect( | ||
| (await page.context().cookies()).find( | ||
| (cookie) => cookie.name === "next-auth.session-token", | ||
| ), | ||
| ).toBeTruthy(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen cookie verification.
The current verification only checks for cookie existence. Consider validating the cookie value matches what was set.
expect(
- (await page.context().cookies()).find(
- (cookie) => cookie.name === "next-auth.session-token",
- ),
- ).toBeTruthy();
+ (await page.context().cookies()).find(
+ (cookie) => cookie.name === "next-auth.session-token" &&
+ cookie.value === sessionId
+ ),
+ ).toBeTruthy("Session cookie was not set correctly");Committable suggestion was skipped due to low confidence.
|
|
||
| export const loggedInAsUserOne = async (page: Page) => { | ||
| try { | ||
| expect(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for environment variable.
Consider adding runtime type checking for the environment variable value, not just its existence.
- expect(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined();
+ const sessionId = process.env.E2E_USER_ONE_SESSION_ID;
+ expect(sessionId).toBeDefined();
+ expect(typeof sessionId === 'string' && sessionId.length > 0).toBeTruthy();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined(); | |
| const sessionId = process.env.E2E_USER_ONE_SESSION_ID; | |
| expect(sessionId).toBeDefined(); | |
| expect(typeof sessionId === 'string' && sessionId.length > 0).toBeTruthy(); |
JohnAllenTech
changed the title
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
drizzle/seed.ts (2)
14-16: Consider updating related environment variables for consistency.While
E2E_USER_ONE_SESSION_IDfollows the new naming pattern for multi-user support, the other E2E variables (E2E_USER_ID,E2E_USER_EMAIL) still use the old pattern. Consider updating these for consistency:-const E2E_USER_ID = - process.env.E2E_USER_ID || "8e3179ce-f32b-4d0a-ba3b-234d66b836ad"; -const E2E_USER_EMAIL = process.env.E2E_USER_EMAIL || "e2e@codu.co"; +const E2E_USER_ID = + process.env.E2E_USER_ONE_ID || "8e3179ce-f32b-4d0a-ba3b-234d66b836ad"; +const E2E_USER_EMAIL = process.env.E2E_USER_ONE_EMAIL || "e2e@codu.co";
12-16: Track technical debt for environment variable cleanup.The comment "These can be removed in a follow on PR" indicates known technical debt. Consider creating a GitHub issue to track the removal of these environment variables in the follow-up PR mentioned in the comment.
Would you like me to create a GitHub issue to track this technical debt?
README.md (1)
176-176: Fix punctuation in the environment variables list.There's an extra period after "testing" that should be removed for consistency with the other list items.
Apply this change:
-- `E2E_USER_ONE_SESSION_ID`: The session id that the user will use to authenticate. +- `E2E_USER_ONE_SESSION_ID`: The session id that the user will use to authenticate🧰 Tools
🪛 LanguageTool
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...for testing. -E2E_USER_ONE_SESSION_ID: The session id that the user will use t...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/workflows/e2e-tests.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
- README.md (2 hunks)
- drizzle/seed.ts (1 hunks)
- e2e/utils/utils.ts (1 hunks)
- sample.env (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sample.env
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/utils/utils.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~176-~176: 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 (2)
drizzle/seed.ts (1)
14-14: LGTM! Environment variable renamed for multi-user support.The change from
E2E_USER_SESSION_IDtoE2E_USER_ONE_SESSION_IDaligns with the PR objective of supporting multiple user personas in E2E tests.README.md (1)
144-146: LGTM! Clear documentation for the renamed environment variable.The documentation clearly explains the purpose of
E2E_USER_ONE_SESSION_IDand provides context about its current usage and future plans for multiple test users. This aligns well with the PR objectives.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NiallJoeMaher
had a problem deploying
to
production
GitHub Actions
Failure
|
Something strange happening when deployed. Will do some troubleshooting. Everything is working locally with the same commands. |
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/utils/utils.ts (1 hunks)
🔇 Additional comments (5)
e2e/utils/utils.ts (5)
1-3: LGTM! Clean imports and well-typed function signature.The function name and type definitions clearly communicate the purpose and requirements.
4-5: Skip comment - existing review covers the type safety concern.
17-21: Skip comment - existing review covers the cookie verification enhancement.
22-26: Skip comment - existing review covers the error handling improvements.
1-27: Verify environment configuration in deployment.Given the reported deployment issues and the dependency on environment variables, let's verify the configuration:
✅ Verification successful
Environment variables are correctly configured for E2E testing
The environment setup is properly documented and configured for E2E testing:
E2E_USER_ONE_SESSION_IDis defined insample.envwith a default value- The variable is documented in README.md
- It's used consistently across the codebase (in
drizzle/seed.tsande2e/utils/utils.ts)- The implementation in
utils.tsincludes proper error handling and validationThe reported deployment issues are likely not related to the environment configuration as the setup appears to be complete and well-structured for E2E testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for environment variable usage and configuration files # Test 1: Search for environment variable references echo "Checking environment variable usage:" rg "E2E_USER_ONE_SESSION_ID" -A 2 # Test 2: Look for environment configuration files echo -e "\nChecking environment configuration files:" fd -g "*.env*" # Test 3: Check deployment configuration for environment setup echo -e "\nChecking deployment configuration:" fd -g "*config*yml" -g "*config*yaml" -g "*config*json"Length of output: 1438
Script:
#!/bin/bash # Let's check the sample.env file content and README.md for environment setup instructions echo "Checking sample.env content:" cat sample.env echo -e "\nChecking README.md for environment setup instructions:" rg -A 5 "Environment Variables|Prerequisites|Getting Started" README.md # Check if this is specifically for E2E testing echo -e "\nChecking E2E test configuration:" fd -g "*playwright.config*"Length of output: 1660
| await page.context().addCookies([ | ||
| { | ||
| name: "next-auth.session-token", | ||
| value: process.env.E2E_USER_ONE_SESSION_ID as string, | ||
| domain: "localhost", | ||
| path: "/", | ||
| sameSite: "Lax", | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Enhance cookie security configuration.
The cookie configuration is missing important security flags:
httpOnlyto prevent XSS attackssecureflag for HTTPS-only transmission
Apply this diff to improve security:
await page.context().addCookies([
{
name: "next-auth.session-token",
value: process.env.E2E_USER_ONE_SESSION_ID as string,
domain: "localhost",
path: "/",
sameSite: "Lax",
+ httpOnly: true,
+ secure: process.env.NODE_ENV === "production"
},
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.context().addCookies([ | |
| { | |
| name: "next-auth.session-token", | |
| value: process.env.E2E_USER_ONE_SESSION_ID as string, | |
| domain: "localhost", | |
| path: "/", | |
| sameSite: "Lax", | |
| }, | |
| ]); | |
| await page.context().addCookies([ | |
| { | |
| name: "next-auth.session-token", | |
| value: process.env.E2E_USER_ONE_SESSION_ID as string, | |
| domain: "localhost", | |
| path: "/", | |
| sameSite: "Lax", | |
| httpOnly: true, | |
| secure: process.env.NODE_ENV === "production" | |
| }, | |
| ]); |
JohnAllenTech
had a problem deploying
to
production
GitHub Actions
Failure
|
@NiallJoeMaher it seems that the PR workflow always runs from the main branches version. I have a new env var thats needed for the tests to pass. Not sure how to work around this.. I am pretty confident it will pass the E2E tests if you merge it though. Can we see and revert if thats not the case? |
|
It needs E2E_USER_ONE_SESSION_ID which i have added to the E2E test workflow file. |
|
It worked anyway! I imagine this will be the same for the other two... maybe |
2 participants
✨ Codu Pull Request 💻
Helps with #1168 and #468
Pull Request details
So eventually we will need multiple user personas in a single E2E test.
One example is a user replying to another users comment. This PR is a first step to support multiple users in our E2E test. Follow on PRs will create a new loggedInAsUserTwo function which will be used to reply to 'user on'
I have also switched from having our tests run as authenticated by default
Any Breaking changes
Associated Screenshots