Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/adding test for replying to a comment #1183

Conversation

JohnAllenTech
Copy link
Contributor

✨ Codu Pull Request 💻

Will need to be merged after #1182

This closes out the multiple E2E user work

Pull Request details

This PR introduces a new test to check a user can reply to another users comment.
In order to isolate this test, we need to prepopulate the DB with an article and a comment already made on it. This is done by the global setup file for the E2E test suite.

Any Breaking changes

  • None

Associated Screenshots

image

image

@JohnAllenTech JohnAllenTech requested a review from a team as a code owner October 26, 2024 22:31
Copy link

vercel bot commented Oct 26, 2024

@JohnAllenTech is attempting to deploy a commit to the Codú Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

coderabbitai bot commented Oct 26, 2024

Walkthrough

The changes in this pull request involve significant updates to the end-to-end (E2E) testing configurations across multiple files. Key modifications include renaming and introducing new environment variables for managing multiple E2E users, enhancing user data management in the seeding process, and updating test cases to accommodate these changes. The setup and teardown scripts have also been introduced and modified to ensure a clean testing environment, while new utility functions have been created to streamline user authentication. Overall, the changes aim to improve the flexibility and scalability of the E2E testing framework.

Changes

File Change Summary
README.md Renamed E2E_USER_SESSION_ID to E2E_USER_ONE_SESSION_ID, added E2E_USER_TWO_SESSION_ID, split E2E_USER_ID into E2E_USER_ONE_ID and E2E_USER_TWO_ID, and updated descriptions for clarity. Minor formatting adjustments made.
drizzle/seed.ts Expanded user setup for two distinct E2E users with new constants. Updated seedE2EUser and seedE2EUserSession functions to accept parameters for dynamic user data creation.
e2e/articles.spec.ts Added new test cases for authenticated and unauthenticated users, including navigation and comment reply functionalities. Updated existing test case for publishing an article to reflect new author naming.
e2e/auth.setup.ts Deleted file containing authentication setup functionality for E2E tests.
e2e/home.spec.ts Introduced loggedInAsUserOne utility function for authenticated tests and removed cookie-clearing step from unauthenticated tests.
e2e/login.spec.ts Added loggedInAsUserOne function to ensure user login before tests. Updated assertions to check for hidden elements for authenticated users.
e2e/my-posts.spec.ts Removed cookie-clearing step from unauthenticated tests and added loggedInAsUserOne for authenticated tests.
e2e/settings.spec.ts Removed cookie-clearing step for unauthenticated tests and added loggedInAsUserOne for authenticated tests.
e2e/setup.ts New file introduced to handle database setup and population for E2E tests, including checks for required environment variables.
e2e/teardown.ts Enhanced environment variable checks and updated cleanup logic to handle multiple E2E users during teardown.
e2e/utils/index.ts Re-exported all exports from the ./utils module for consolidated access.
e2e/utils/utils.ts Introduced loggedInAsUserOne and loggedInAsUserTwo functions for user authentication in E2E tests.
playwright.config.ts Added globalSetup property pointing to ./e2e/setup.ts, removed storageState and dependencies properties from various project configurations.
sample.env Updated environment variables to support two distinct E2E users, replacing single user variables with E2E_USER_ONE_ID, E2E_USER_TWO_ID, and their corresponding session IDs.

Possibly related PRs

Suggested labels

hacktoberfest-accepted, hacktoberfest

Suggested reviewers

  • NiallJoeMaher

Poem

In the fields where rabbits play,
New tests hop in, brightening the day.
With users two, we leap and bound,
In E2E joy, our changes are found!
So let’s celebrate with a twitch of our nose,
For testing is fun, as everyone knows! 🐰✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

🧹 Outside diff range and nitpick comments (9)
e2e/my-posts.spec.ts (1)

1-15: Consider adding test coverage for edge cases.

Since this file is part of the E2E test suite for replying to comments (as per PR objectives), consider adding test cases for:

  • Comment reply functionality for both authenticated and unauthenticated users
  • Error handling for invalid replies
  • Rate limiting scenarios
sample.env (1)

8-12: Add documentation for E2E testing variables.

Consider adding comments to explain the purpose of these E2E variables and their expected format.

Here's a suggested improvement:

 [email protected]
+# UUID v4 identifiers for E2E test users
+# Used in end-to-end tests for testing user interactions (e.g., comment replies)
 E2E_USER_ONE_ID=8e3179ce-f32b-4d0a-ba3b-234d66b836ad
 E2E_USER_TWO_ID=a15a104a-0e34-4101-8800-ed25c9231345
+# Session IDs for authenticating E2E test users
+# Format: UUID v4
 E2E_USER_ONE_SESSION_ID=df8a11f2-f20a-43d6-80a0-a213f1efedc1
 E2E_USER_TWO_SESSION_ID=10134766-bc6c-4b52-83d7-46ec0a4cb95d
e2e/teardown.ts (1)

Line range hint 1-38: Add detailed logging for debugging.

For E2E testing purposes, it would be helpful to know how many records were deleted during cleanup.

Consider adding RETURNING count to your DELETE queries and logging the results:

     await db.begin(async (db) => {
-      await db`
+      const deletedPosts = await db`
         DELETE FROM "Post" 
         WHERE "userId" IN (${process.env.E2E_USER_ONE_ID as string}, ${process.env.E2E_USER_TWO_ID as string})
+        RETURNING count(*) as count
       `;
       
-      await db`
+      const deletedComments = await db`
         DELETE FROM "Comment" 
         WHERE "userId" IN (${process.env.E2E_USER_ONE_ID as string}, ${process.env.E2E_USER_TWO_ID as string})
+        RETURNING count(*) as count
       `;
+      
+      console.log(`Cleanup completed: ${deletedPosts[0].count} posts and ${deletedComments[0].count} comments deleted`);
     });
e2e/utils/utils.ts (1)

1-51: Add JSDoc documentation to explain the purpose and usage of these functions.

Since these functions are crucial for testing user interactions, particularly for the comment reply functionality mentioned in the PR objectives, consider adding comprehensive documentation:

/**
 * Authentication utilities for E2E testing with multiple users.
 * These functions manage session cookies to simulate authenticated users
 * interacting with features like comment replies.
 */

/**
 * Authenticates as the first test user by setting up their session cookie.
 * Used in scenarios where a single authenticated user is needed.
 * @param page - The Playwright page object
 * @throws Error if the session ID environment variable is not configured
 */
export const loggedInAsUserOne = ...

/**
 * Authenticates as the second test user by setting up their session cookie.
 * Used in scenarios requiring interaction between multiple users,
 * such as commenting and replying to comments.
 * @param page - The Playwright page object
 * @throws Error if the session ID environment variable is not configured
 */
export const loggedInAsUserTwo = ...
e2e/login.spec.ts (1)

Line range hint 38-60: Consider small improvements to test maintainability

The test logic is solid and correctly verifies authentication behavior. Consider these minor improvements:

  1. Extract the base URL into a constant to avoid repetition
  2. Add a brief comment explaining the redirect behavior for future maintainers

Example refactor:

+ const BASE_URL = 'http://localhost:3000';
+ const GET_STARTED_PATH = '/get-started';

  test("Sign up page contains sign up links", async ({ page, isMobile }) => {
+   // Authenticated users should be redirected to homepage when accessing the get-started page
-   await page.goto("http://localhost:3000/get-started");
-   expect(page.url()).toEqual("http://localhost:3000/");
+   await page.goto(`${BASE_URL}${GET_STARTED_PATH}`);
+   expect(page.url()).toEqual(`${BASE_URL}/`);
drizzle/seed.ts (3)

13-25: Consider enhancing environment variable handling.

A few suggestions for improvement:

  1. Create a tracking issue for removing the hardcoded defaults as mentioned in the comment.
  2. Consider using a UUID generator for default values instead of hardcoded UUIDs to prevent potential collisions.
  3. Add type safety by creating an environment variable schema using a library like zod.

Would you like me to help create a tracking issue or demonstrate how to implement these improvements?


Line range hint 127-152: Add input validation and prevent SQL injection.

The function needs additional safeguards:

  1. Validate email format before database insertion
  2. Add minimum length validation for name parameter
  3. Sanitize inputs used in username generation to prevent SQL injection

Here's a suggested implementation:

 const seedE2EUser = async (email: string, id: string, name: string) => {
+  // Validate email format
+  if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) {
+    throw new Error('Invalid email format');
+  }
+
+  // Validate name length
+  if (name.length < 2) {
+    throw new Error('Name must be at least 2 characters long');
+  }
+
   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;
   }

+  // Sanitize username input
+  const sanitizedName = name.replace(/[^a-zA-Z0-9-]/g, '');
   const userData = {
     id: id,
-    username: `${name.split(" ").join("-").toLowerCase()}-${chance.integer({
+    username: `${sanitizedName.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;
 };

Line range hint 155-180: Improve error handling and configuration.

The function needs the following improvements:

  1. Better error handling instead of just logging
  2. Validation for sessionToken format
  3. Session expiry duration should be configurable

Here's a suggested implementation:

+ const SESSION_EXPIRY_MONTHS = 6; // Move to configuration
+
  const seedE2EUserSession = async (userId: string, sessionToken: string) => {
+   if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(sessionToken)) {
+     throw new Error('Invalid session token format. Expected UUID v4.');
+   }
+
    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();
+     const expiryDate = new Date(currentDate);
+     expiryDate.setMonth(currentDate.getMonth() + SESSION_EXPIRY_MONTHS);

      return await db
        .insert(session)
        .values({
          userId,
          sessionToken,
-         // Set session to expire in 6 months.
-         expires: new Date(currentDate.setMonth(currentDate.getMonth() + 6)),
+         expires: expiryDate,
        })
        .returning();
    } catch (err) {
-     console.log(err);
+     throw new Error(`Failed to create session: ${err.message}`);
    }
  };
e2e/articles.spec.ts (1)

17-30: Consider making the test article slug configurable

While the test effectively verifies article navigation and content visibility, the hardcoded slug e2e-test-slug-eqj0ozor could make the test fragile. Consider moving this to a configuration file or environment variable for better maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5a60d53 and ce0cb8e.

⛔ Files ignored due to path filters (2)
  • .github/workflows/e2e-tests.yml is excluded by !**/*.yml
  • playwright/.auth/browser.json is excluded by !**/*.json
📒 Files selected for processing (14)
  • README.md (2 hunks)
  • drizzle/seed.ts (5 hunks)
  • e2e/articles.spec.ts (5 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/setup.ts (1 hunks)
  • e2e/teardown.ts (1 hunks)
  • e2e/utils/index.ts (1 hunks)
  • e2e/utils/utils.ts (1 hunks)
  • playwright.config.ts (1 hunks)
  • sample.env (1 hunks)
💤 Files with no reviewable changes (1)
  • e2e/auth.setup.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.
🪛 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 (20)
e2e/my-posts.spec.ts (2)

1-2: LGTM! Clean import structure.

The import of the authentication utility is properly organized and follows best practices.


4-7: Implementation needed: Add tests for unauthenticated scenarios.

The test suite is currently empty. Consider adding test cases for:

  • Redirecting unauthenticated users to login
  • Displaying appropriate error messages
  • Handling unauthorized access attempts

Would you like me to help generate test cases for the unauthenticated scenarios?

e2e/settings.spec.ts (3)

10-12: LGTM! Authentication setup looks good

The beforeEach hook correctly sets up authentication using the new loggedInAsUserOne utility function, which aligns with the PR's objective of supporting multiple E2E users.


13-15: Replace placeholder comment with actual test implementation

The placeholder comment should be replaced with actual test cases for authenticated users. Consider testing scenarios like:

  • Accessing and modifying user settings
  • Form validation
  • Success/error messages
#!/bin/bash
# Check if similar tests exist in other files that we can reference
rg -l 'test\.describe.*[Aa]uthenticated' 'e2e/'

Would you like me to help generate the test implementations for authenticated scenarios?


4-8: Replace placeholder comment with actual test implementation

The placeholder comment should be replaced with actual test cases for unauthenticated users. Consider testing scenarios like:

  • Redirecting to login page when accessing protected settings
  • Displaying appropriate error messages
  • Verifying restricted access to settings features

Would you like me to help generate the test implementations for unauthenticated scenarios?

✅ Verification successful

Add unauthenticated settings tests following existing patterns

Based on the codebase patterns, implement tests for unauthenticated users in the settings page similar to other files:

  • Test redirection to homepage or login page when accessing settings
  • Add test.beforeEach to clear cookies like in login.spec.ts
  • Follow similar structure as articles.spec.ts and home.spec.ts for unauthenticated scenarios

Example implementation:

test.describe("Unauthenticated settings Page", () => {
  test.beforeEach(async ({ page }) => {
    await page.context().clearCookies();
    await page.goto("http://localhost:3000/settings");
  });

  test("Should redirect unauthenticated users", async ({ page }) => {
    await expect(page).toHaveURL("http://localhost:3000/get-started");
  });
});
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if similar tests exist in other files that we can reference
rg -l 'test\.describe.*[Uu]nauthenticated' 'e2e/'

Length of output: 239


Script:

#!/bin/bash
# Let's check the implementation of unauthenticated tests in these files
rg -B2 -A5 'test\.describe.*[Aa]uthenticated' e2e/my-posts.spec.ts e2e/login.spec.ts e2e/home.spec.ts e2e/articles.spec.ts

Length of output: 3449

sample.env (1)

9-12: LGTM! The UUIDs are properly formatted and unique.

The new user and session IDs are correctly structured as version 4 UUIDs and maintain uniqueness across all variables.

Let's verify the uniqueness of these UUIDs across the codebase:

✅ Verification successful

UUIDs are properly used and synchronized with seed data

The verification confirms that:

  • Each UUID appears exactly twice in the codebase
  • One occurrence in sample.env and another as a fallback value in drizzle/seed.ts
  • No duplicate or conflicting usage found
  • The values are consistently used across both configuration files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any duplicate UUIDs in the codebase
# Test: Search for the exact UUIDs. Expect: Only one occurrence per UUID

for uuid in "8e3179ce-f32b-4d0a-ba3b-234d66b836ad" "a15a104a-0e34-4101-8800-ed25c9231345" "df8a11f2-f20a-43d6-80a0-a213f1efedc1" "10134766-bc6c-4b52-83d7-46ec0a4cb95d"; do
    echo "Checking UUID: $uuid"
    rg "$uuid"
done

Length of output: 1943

e2e/home.spec.ts (2)

1-2: LGTM! Clean import of authentication utility.

The addition of the centralized authentication utility aligns well with the PR's objective of improving E2E test structure.


5-7: Verify the implementation of loggedInAsUserOne utility.

The authentication setup looks good, but let's verify the implementation of the utility function to ensure consistent behavior across tests.

✅ Verification successful

The loggedInAsUserOne utility is well-implemented and consistently used across test files

The utility follows best practices by:

  • Using environment variables for session management
  • Properly setting up cookie attributes (domain, path, sameSite)
  • Having consistent usage pattern across all e2e test files (articles, home, login, my-posts, settings)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of loggedInAsUserOne utility
# and its usage across test files

# Check the utility implementation
echo "Checking loggedInAsUserOne implementation:"
rg -A 10 "export.*loggedInAsUserOne" e2e/

# Check usage patterns across test files
echo "\nChecking usage patterns:"
rg "loggedInAsUserOne" e2e/

Length of output: 1461

e2e/utils/utils.ts (1)

1-2: LGTM! Imports are appropriate for Playwright testing.

playwright.config.ts (2)

13-14: Good architectural improvement with centralized setup/teardown!

Moving from per-browser auth state to a global setup/teardown approach is a good architectural decision that will:

  • Centralize the test data initialization
  • Make the configuration more maintainable
  • Reduce duplicate auth logic across browser configurations

Line range hint 13-19: Verify parallel test execution with new setup

The configuration enables full parallel execution (fullyParallel: true) with the new global setup. Please ensure that:

  1. The global setup properly handles concurrent test execution
  2. The database seeding is atomic and thread-safe
  3. Test isolation is maintained across parallel runs

Let's verify the setup script handles these concerns:

✅ Verification successful

Parallel test execution is safely implemented

The verification shows that the test setup is properly designed for parallel execution:

  1. The global setup (setup.ts) uses atomic database operations:

    • Uses onConflictDoNothing() for inserts to handle concurrent operations
    • Creates test data with unique IDs and fixed data
    • Scopes data to specific test users (E2E_USER_ONE_ID, E2E_USER_TWO_ID)
  2. The teardown (teardown.ts) maintains isolation:

    • Cleans up only test-specific data using user IDs
    • Uses separate DELETE statements to avoid transaction conflicts
  3. Test files show proper isolation:

    • Each test suite uses test.beforeEach() for setup
    • Tests operate on isolated data through authenticated user contexts
    • No direct database operations in test files, only UI interactions
    • No test dependencies on execution order

The parallel execution configuration is safe to use with this setup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if setup.ts implements proper isolation and concurrency handling

# Look for database transaction or isolation mechanisms
echo "Checking for transaction handling in setup..."
rg -A 5 "transaction|prisma" "e2e/setup.ts"

# Look for cleanup mechanisms
echo "Checking for cleanup mechanisms..."
rg "beforeEach|afterEach|beforeAll|afterAll" "e2e/setup.ts"

# Check for race condition prevention
echo "Checking for race condition handling..."
rg "lock|mutex|synchronized" "e2e/setup.ts"

Length of output: 416


Script:

#!/bin/bash
# Let's first check if the setup file exists and see its content
echo "Checking if setup.ts exists and its content:"
cat e2e/setup.ts

# Also check teardown.ts as it's part of the test setup
echo -e "\nChecking teardown.ts content:"
cat e2e/teardown.ts

# Look for test files to understand the testing patterns
echo -e "\nListing test files:"
fd -e test.ts -e spec.ts . e2e/

# Check for database configuration files
echo -e "\nChecking for database config:"
fd -g "*database*" -g "*db*" . e2e/

Length of output: 4109


Script:

#!/bin/bash
# Let's check the test files for any concurrent operations or database interactions
echo "Checking test files for database operations and concurrent tests:"
rg "test\(.*async" e2e/*.spec.ts -A 2

# Look for any describe blocks that might run in parallel
echo -e "\nChecking for test suites:"
rg "describe\(" e2e/*.spec.ts -A 2

# Check if any tests are explicitly marked as serial
echo -e "\nChecking for serial test annotations:"
rg "serial|sequential|@serial" e2e/*.spec.ts

# Look for database operations in test files
echo -e "\nChecking for database operations in tests:"
rg "insert|update|delete|query" e2e/*.spec.ts

Length of output: 8980

e2e/login.spec.ts (2)

3-3: LGTM: Clean import addition

The import is well-organized and follows proper conventions.


35-37: LGTM: Good test setup practice

The beforeEach hook ensures proper test isolation by establishing a consistent authenticated state.

Let's verify the implementation of the utility function:

✅ Verification successful

Let me check the implementation of the utility function since we found several files using it.


Let me check the complete implementation since it was cut off in the previous output.


Utility implementation verified and properly handles authentication

The loggedInAsUserOne utility is well-implemented with proper:

  • Session token validation via environment variable
  • Cookie management using next-auth session
  • Error handling and assertions
  • Type safety with Playwright's Page type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the loggedInAsUserOne utility implementation
# Expected: Find the implementation and verify it handles authentication properly

# Search for the utility implementation
echo "Searching for loggedInAsUserOne implementation..."
ast-grep --pattern 'export const loggedInAsUserOne = async ($page) => { $$$ }'

# Search for any related test utilities
echo "Checking for related test utilities..."
rg -l "loggedInAsUserOne" --type ts

Length of output: 429


Script:

#!/bin/bash
# Get the implementation from utils.ts
echo "Content of loggedInAsUserOne in utils.ts:"
rg "loggedInAsUserOne" e2e/utils/utils.ts -A 10

# Check if there are any login-related utilities
echo -e "\nRelated login utilities:"
rg "login|auth" e2e/utils/utils.ts

Length of output: 931


Script:

#!/bin/bash
# Get the complete implementation
echo "Complete implementation of loggedInAsUserOne:"
cat e2e/utils/utils.ts

Length of output: 1434

e2e/setup.ts (3)

1-6: LGTM! Imports and environment setup are well structured.

The necessary dependencies are properly imported and environment variables are loaded correctly.


65-65: LGTM! Export statement is correct.

The default export is properly implemented.


41-50: Verify comment schema completeness for reply functionality.

Since this setup is specifically for testing comment replies, we should ensure all necessary fields for the reply feature are included in the comment creation.

Let's check the comment schema and existing reply implementations:

✅ Verification successful

Let me try another search to find the schema definition since it might be in a different location:


Comment schema is complete for reply functionality

The schema and implementation support reply functionality through the parentId field in the comment table, which is correctly used in the test setup. The current schema includes all necessary fields:

  • postId - links to the parent post
  • body - comment content
  • userId - commenter's ID
  • parentId - optional field for reply relationships

The test setup creates a base comment that can be replied to, which is sufficient for testing the reply feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check comment schema and existing implementations
echo "Checking comment schema..."
rg -A 10 "comment =" "server/db/schema"

echo "Checking existing reply implementations..."
rg -A 5 "reply" "app/"

Length of output: 18355


Script:

#!/bin/bash
# Search for comment schema definition
echo "Searching for comment schema definition..."
fd -e ts -e js schema
rg -A 10 "comment.*schema|comment.*table" 

# Search for reply-related functionality
echo "Searching for reply functionality implementation..."
rg -A 10 "replyTo|parentId|parent.*comment"

Length of output: 32157

e2e/articles.spec.ts (4)

3-4: LGTM: Clean utility import

The import of loggedInAsUserOne utility aligns with the best practices of keeping authentication logic centralized and reusable.


148-150: LGTM: Clean authentication setup

The beforeEach hook properly centralizes the authentication setup for all tests in this describe block, following the best practices mentioned in the retrieved learning.


269-269: LGTM: Consistent user identifier update

The heading text update aligns with the standardized user identifiers mentioned in the AI summary.


293-333: ⚠️ Potential issue

Several improvements needed in the reply test

The test has the following issues that should be addressed:

  1. Using waitForTimeout is an anti-pattern in E2E tests
  2. Comment counting logic is fragile due to text-based filtering
  3. Typo in variable name "numberOfCommentsAfteringCommenting"
  4. Hardcoded test data dependencies

Consider these improvements:

 test("Should be able reply to a comment", async ({ page }) => {
   await page.goto("http://localhost:3000/articles/e2e-test-slug-eqj0ozor");
-  const numberOfCommentsIntially = await page
-    .locator("div")
-    .filter({ hasText: /^Thanks for the positive feedback!$/ })
-    .count();
+  const initialCommentCount = await page.getByTestId("comment-item").count();
   
   // ... visibility checks ...
   
   await page.getByRole("button", { name: "Reply" }).first().click();
   await page.locator("#reply").fill("Thanks for the positive feedback!");
   await page.getByRole("button", { name: "Submit" }).nth(1).click();
-  await page.waitForTimeout(250);
+  // Wait for the new comment to appear
+  await expect(page.getByTestId("comment-item")).toHaveCount(initialCommentCount + 1);
   
   await expect(
     page.getByText("AUTHOR", { exact: true }).first(),
   ).toBeVisible();
-  const numberOfCommentsAfteringCommenting = await page
-    .locator("div")
-    .filter({ hasText: /^Thanks for the positive feedback!$/ })
-    .count();
-  expect(numberOfCommentsAfteringCommenting).toBeGreaterThan(
-    numberOfCommentsIntially,
-  );
   await expect(
     page
       .getByRole("link", { name: "E2E Test User One", exact: true })
-      .nth(numberOfCommentsIntially + 1),
+      .nth(initialCommentCount + 1),
   ).toBeVisible();
 });

Additionally:

  1. Add a data-testid attribute to comment elements for more reliable selection
  2. Consider moving the test article slug to a configuration file

Let's verify the usage of data-testid attributes in the codebase:

e2e/my-posts.spec.ts Outdated Show resolved Hide resolved
e2e/settings.spec.ts Outdated Show resolved Hide resolved
e2e/teardown.ts Outdated Show resolved Hide resolved
e2e/teardown.ts Outdated Show resolved Hide resolved
e2e/teardown.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@JohnAllenTech
Copy link
Contributor Author

This is the same job @NiallJoeMaher it wont work because its using the main branches e2e workflow file but the tests require a new env variable.

If its merged I am reasonably sure it will work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants