chore: removed all e2e env variables and migrated them to constants file #1189
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 involve significant modifications to the end-to-end (E2E) testing documentation and related code files. The "Environment Variables" section has been removed from the documentation, while the "Additional E2E Environment Variables" section has been renamed and updated to include new constants. Several constants related to E2E users have been added or redefined across multiple files, transitioning from hardcoded values to a centralized constants module. This restructuring aims to simplify the management of user data and enhance the maintainability of the codebase. 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
temporarily deployed
to
production
GitHub Actions
Inactive
![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 (11)
E2E Overview.md (3)
Line range hint 1-32: Update setup instructions to reflect constants-based approach.
The setup section should be updated to reference the new constants-based approach and provide guidance on where these constants are defined.
Add a note after the seed script section:
npm run db:seed-This will create a user and session for your E2E tests. Details of the E2E user created can be seen in drizzle/seedE2E.ts.
+This will create users and sessions for your E2E tests. The user details are defined in e2e/constants and used in drizzle/seedE2E.ts.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 LanguageTool</summary>
[style] ~37-~37: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...on token UUID for another E2E user. - **E2E_USER_ONE_ID**: The user ID of one of th...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
---
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...he user ID of one of the E2E users. - **E2E_USER_TWO_ID**: The user ID of another E...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
</details>
</details>
---
`33-33`: **Enhance constants documentation with usage examples.**
While the constants are well documented, adding their location and example usage would make the documentation more helpful.
Add the following details:
```diff
### Additional E2E constants
+These constants are defined in `e2e/constants` and should be used instead of environment variables in your E2E tests.
+
- **E2E_USER_ONE_SESSION_ID**: This is the session token UUID for one E2E user.
- **E2E_USER_TWO_SESSION_ID**: This is the session token UUID for another E2E user.
- **E2E_USER_ONE_ID**: The user ID of one of the E2E users.
- **E2E_USER_TWO_ID**: The user ID of another E2E user.
+
+Example usage in tests:
+```typescript
+import { E2E_USER_ONE_SESSION_ID } from '../constants';
+
+test('user can view their profile', async ({ page }) => {
+ // Set the session cookie using the constant
+ await page.context().addCookies([
+ {
+ name: 'session',
+ value: E2E_USER_ONE_SESSION_ID,
+ domain: 'localhost',
+ path: '/',
+ },
+ ]);
+});
+```
Environment variable references found in package.json scripts
The documentation has been properly updated to remove environment variable references. However, there are still environment variable usages in package.json:
"dev:e2e": "ENV=E2E next dev""test:e2e:ui": "ENV=local npx playwright test --ui"
These script configurations should be reviewed to ensure they align with the documentation changes and current E2E testing approach.
🔗 Analysis chain
Line range hint 1-33: Verify complete removal of environment variable references.
Let's ensure all E2E environment variable references have been removed from the documentation and related files.
Let me try a different search strategy to verify the removal of environment variables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining E2E environment variable references
# Test 1: Search for E2E env variable references in documentation
echo "Checking documentation for env references..."
rg -i "E2E.*env" docs/
# Test 2: Search for process.env.E2E in test files
echo "Checking test files for process.env.E2E..."
rg "process\.env\.E2E" e2e/
# Test 3: Check for E2E variables in .env files
echo "Checking .env files for E2E variables..."
rg "^E2E_" .env* || true
Length of output: 459
Script:
#!/bin/bash
# Search for environment variable references in all files
echo "Searching for E2E environment variables in all files..."
rg -i "env.*e2e|e2e.*env"
# Search specifically in markdown files
echo -e "\nSearching in markdown files..."
fd -e md -x rg -i "env.*e2e|e2e.*env" {}
# Check for environment setup instructions
echo -e "\nChecking for environment setup mentions..."
rg -i "environment.*setup|setup.*environment" "E2E Overview.md"
Length of output: 555
🧰 Tools
🪛 LanguageTool
[style] ~37-~37: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...on token UUID for another E2E user. - E2E_USER_ONE_ID: The user ID of one of th...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...he user ID of one of the E2E users. - E2E_USER_TWO_ID: The user ID of another E...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
e2e/utils/utils.ts (1)
33-33: LGTM! Consider adding cookie clearing to loggedInAsUserOne for consistency.
The change successfully removes the environment variable dependency. The addition of clearCookies() in this function is a good practice to prevent authentication conflicts.
Consider adding the same cookie clearing to loggedInAsUserOne for consistency:
export const loggedInAsUserOne = async (page: Page) => {
try {
+ await page.context().clearCookies();
await page.context().addCookies([e2e/constants/constants.ts (1)
10-12: Document the purpose of each test user.
Consider adding JSDoc comments to explain the intended use case for each test user, making it easier for other developers to understand when to use each one in their tests.
+/**
+ * Test User One: Primary user for basic authentication and authorization tests
+ */
export const E2E_USER_ONE_EMAIL = "[email protected]";
export const E2E_USER_ONE_ID: UUID = "8e3179ce-f32b-4d0a-ba3b-234d66b836ad";
export const E2E_USER_ONE_SESSION_ID: UUID = "df8a11f2-f20a-43d6-80a0-a213f1efedc1";
+/**
+ * Test User Two: Secondary user for testing user interactions, permissions, and multi-user scenarios
+ */
export const E2E_USER_TWO_EMAIL = "[email protected]";
export const E2E_USER_TWO_ID: UUID = "a15a104a-0e34-4101-8800-ed25c9231345";
export const E2E_USER_TWO_SESSION_ID: UUID = "10134766-bc6c-4b52-83d7-46ec0a4cb95d";e2e/setup.ts (1)
Line range hint 11-46: Consider extracting test data to fixtures.
The function contains hardcoded test data (post content, IDs, etc.). This could make tests harder to maintain and understand.
Consider:
- Moving test data to separate fixture files
- Creating helper functions for different test scenarios
- Making the test data more configurable
Example structure:
// fixtures/posts.ts
export const TEST_POST = {
id: "1nFnMmN5",
excerpt: "Lorem ipsum dolor sit amet",
// ... other fields
};
// fixtures/comments.ts
export const TEST_COMMENT = {
body: "What a great article! Thanks for sharing",
// ... other fields
};drizzle/seed.ts (5)
10-17: Consider using path aliases for imports.
While the imports correctly centralize E2E constants, using relative paths (../e2e/constants) can become maintenance overhead if the file structure changes. Consider implementing path aliases (e.g., @e2e/constants) for more maintainable imports.
Line range hint 124-146: Add error handling to seedE2EUser function.
The seedE2EUser function lacks error handling compared to seedE2EUserSession. This inconsistency could mask database errors during user creation.
Apply this diff to add error handling:
const seedE2EUser = async (email: string, id: string, name: string) => {
+ try {
const [existingE2EUser] = await db
.selectDistinct()
.from(user)
.where(eq(user.id, id));
if (existingE2EUser) {
console.log("E2E Test user already exists. Skipping creation");
return existingE2EUser;
}
const userData = {
id: id,
username: `${name.split(" ").join("-").toLowerCase()}-${chance.integer({
min: 0,
max: 999,
})}`,
name,
email,
image: `https://robohash.org/${encodeURIComponent(name)}?bgset=bg1`,
location: chance.country({ full: true }),
bio: chance.sentence({ words: 10 }),
websiteUrl: chance.url(),
};
const [createdUser] = await db.insert(user).values(userData).returning();
return createdUser;
+ } catch (err) {
+ console.error("Error creating E2E user:", err);
+ throw err;
+ }
};Line range hint 235-251: Wrap E2E user and session creation in a transaction.
The E2E user and session creation are related operations but not wrapped in a transaction. This could lead to orphaned sessions if user creation fails.
Apply this diff to add transaction support:
try {
+ await db.transaction(async (tx) => {
await addUserData();
const userOne = await seedE2EUser(
E2E_USER_ONE_EMAIL,
E2E_USER_ONE_ID,
"E2E Test User One",
);
const userTwo = await seedE2EUser(
E2E_USER_TWO_EMAIL,
E2E_USER_TWO_ID,
"E2E Test User Two",
);
await seedE2EUserSession(userOne.id, E2E_USER_ONE_SESSION_ID);
await seedE2EUserSession(userTwo.id, E2E_USER_TWO_SESSION_ID);
+ });
} catch (error) {
- console.log("Error:", error);
+ console.error("Error seeding E2E data:", error);
+ throw error;
}Line range hint 253-289: Fix inconsistent logging and improve SQL safety in deleteDataFromAllTables.
The function has several issues:
- It logs both "Delete" and "Skipping" for the same table.
- Uses raw SQL execution which could be unsafe.
Apply this diff to fix the issues:
async function deleteDataFromAllTables() {
- const query = sql<string>`SELECT table_name
- FROM information_schema.tables
- WHERE table_schema = 'public'
- AND table_type = 'BASE TABLE';
- `;
+ const tables = ['user', 'post', 'tag', 'like', 'post_tag', 'session'];
- const tables = await db.execute(query); // retrieve tables
-
for (const table of tables) {
try {
- const query =
- await sql`DELETE FROM "${sql.raw(table.table_name as string)}" CASCADE;`;
- await db.execute(query);
- console.log("Delete", table.table_name);
- console.log(`Skipping ${table.table_name}`);
+ await db.delete(table);
+ console.log(`Deleted all records from ${table}`);
} catch (error) {
- console.log(`Error deleting ${table.table_name}: ${error}`);
+ console.error(`Error deleting from ${table}:`, error);
}
}
console.log(`Database emptied`);
}Line range hint 291-299: Add warning message before database deletion.
The production check is good, but it would be helpful to add a warning message before proceeding with database deletion in development.
Apply this diff to add a warning:
if (process.env.NODE_ENV !== "production") {
+ console.warn(
+ "⚠️ Warning: This will delete all data from the database. Press Ctrl+C to cancel..."
+ );
+ // Give 5 seconds to cancel
+ await new Promise(resolve => setTimeout(resolve, 5000));
await deleteDataFromAllTables();
await addSeedDataToDb();
} else {📜 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 (8)
- E2E Overview.md (1 hunks)
- drizzle/seed.ts (1 hunks)
- e2e/constants/constants.ts (1 hunks)
- e2e/constants/index.ts (1 hunks)
- e2e/setup.ts (2 hunks)
- e2e/teardown.ts (1 hunks)
- e2e/utils/utils.ts (2 hunks)
- sample.env (0 hunks)
💤 Files with no reviewable changes (1)
- sample.env
✅ Files skipped from review due to trivial changes (1)
- e2e/constants/index.ts
🔇 Additional comments (7)
e2e/teardown.ts (2)
1-2: LGTM! Import changes align with PR objectives.
The migration from environment variables to constants is implemented correctly here.
16-21: 🛠️ Refactor suggestion
Optimize comment deletion and verify constraints.
Similar to the post deletion, these queries could be optimized. Additionally, we should verify the correct order of operations if there are foreign key constraints between posts and comments.
Let's verify the table constraints:
Suggested optimization:
- await db`
- DELETE FROM "Comment" WHERE "userId" = ${E2E_USER_ONE_ID}
- `;
- await db`
- DELETE FROM "Comment" WHERE "userId" = ${E2E_USER_TWO_ID}
- `;
+ await db.begin(async (tx) => {
+ await tx`DELETE FROM "Comment" WHERE "userId" IN (${E2E_USER_ONE_ID}, ${E2E_USER_TWO_ID})`;
+ });✅ Verification successful
Let me check for any foreign key constraints specifically:
Transaction optimization is safe to implement
The foreign key constraints all use ON DELETE CASCADE, which means deleting comments will automatically handle dependent records in Like, Flagged, and Notification tables. The self-referential constraint for nested comments (parentId) also cascades, so the deletion order is handled automatically by the database.
- await db`
- DELETE FROM "Comment" WHERE "userId" = ${E2E_USER_ONE_ID}
- `;
- await db`
- DELETE FROM "Comment" WHERE "userId" = ${E2E_USER_TWO_ID}
- `;
+ await db.begin(async (tx) => {
+ await tx`DELETE FROM "Comment" WHERE "userId" IN (${E2E_USER_ONE_ID}, ${E2E_USER_TWO_ID})`;
+ });🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for foreign key constraints between Post and Comment tables
rg -A 10 'CREATE TABLE.*"Comment"'
Length of output: 747
Script:
#!/bin/bash
# Check for foreign key constraints related to Comment table
rg -A 5 'FOREIGN KEY.*Comment' drizzle/
rg -A 5 'REFERENCES.*Comment' drizzle/
Length of output: 7280
e2e/utils/utils.ts (2)
1-2: LGTM! Import changes align with PR objectives.
The addition of imports from the constants file effectively supports the migration away from environment variables.
9-9: LGTM! Verify constant definition.
The change successfully removes the environment variable dependency. The cookie configuration maintains security best practices with appropriate sameSite policy.
Let's verify the constant is properly defined:
✅ Verification successful
Constant is properly defined and exported
The constant E2E_USER_ONE_SESSION_ID is correctly defined and exported in e2e/constants/constants.ts with a valid UUID value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify E2E_USER_ONE_SESSION_ID constant exists and is properly exported
ast-grep --pattern 'export const E2E_USER_ONE_SESSION_ID = $_'
Length of output: 314
e2e/constants/constants.ts (1)
1-12: Verify complete migration from environment variables.
Let's ensure all E2E environment variables have been properly migrated to constants.
✅ Verification successful
Migration to constants is complete and properly used
The verification shows:
- No remaining
process.envreferences for E2E variables in the codebase - Constants are actively used in multiple e2e test files:
- User constants in
teardown.ts,setup.ts, andutils/utils.ts - Article constants in
articles.spec.ts
- User constants in
- No legacy environment variables found in
.envfiles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all E2E environment variables have been migrated to constants
# Test 1: Check for any remaining process.env references in E2E tests
echo "Checking for remaining process.env references in E2E tests..."
rg "process\.env\.(E2E_USER|ARTICLE)" -g "e2e/**/*"
# Test 2: Verify usage of new constants
echo "Verifying usage of new constants..."
rg "E2E_USER_(ONE|TWO)_(EMAIL|ID|SESSION_ID)" -g "e2e/**/*"
# Test 3: Check for any .env files that might contain old variables
echo "Checking .env files for old variables..."
fd -g ".env*" --exec grep -l "E2E_USER"
Length of output: 2000
Script:
#!/bin/bash
# Let's check for any usage of the article-related constants and also check hidden files
# Test 1: Check usage of article constants
echo "Checking usage of article constants..."
rg "articleContent|articleExcerpt" -g "e2e/**/*"
# Test 2: Check hidden env files for E2E variables
echo "Checking hidden env files..."
fd -H -g ".env*" --exec grep -l "E2E_USER"
# Test 3: Double check for any process.env references in e2e tests with more context
echo "Checking process.env references with context..."
rg "process\.env" -g "e2e/**/*" -C 2
Length of output: 633
e2e/setup.ts (2)
4-4: LGTM! Import of constants aligns with PR objectives.
The migration from environment variables to imported constants follows the intended refactoring goal.
48-48: Verify the imported constants are properly defined.
The change from environment variables to constants looks good, but let's verify the constants are properly defined.
✅ Verification successful
Constants are properly defined and used consistently
The constants E2E_USER_ONE_ID and E2E_USER_TWO_ID are correctly defined in e2e/constants/constants.ts with valid UUID values and are imported and used consistently across the e2e test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the E2E user ID constants are properly defined
# Expected: Find the constant definitions in the constants file
# Look for the constant definitions
echo "Searching for E2E user ID constant definitions..."
rg "E2E_USER_(ONE|TWO)_ID" --type ts
Length of output: 1040
✨ Codu Pull Request 💻
Fixes a comment that @NiallJoeMaher made on stream.
No need for these hardcoded E2E env variables. What the heck was I thinking!
Pull Request details
Any Breaking changes
Associated Screenshots
[Optional] What gif best describes this PR or how it makes you feel