Skip to content

Feat/adding test for replying to a comment #1183

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 28, 2024
+123 −0
Merged

Feat/adding test for replying to a comment #1183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
02aabfe
chore: moving out from user auth out into a util
JohnAllenTech Oct 26, 2024
984b47d
chore: missed some updates in the README
JohnAllenTech Oct 26, 2024
168f007
chore: split out e2e user creation/seeding to support two e2e users i…
JohnAllenTech Oct 26, 2024
f023902
feat: added a test to check a user can reply to a comment
JohnAllenTech Oct 26, 2024
ce0cb8e
chore: removing a check to prevent a flaky test
JohnAllenTech Oct 26, 2024
420d9cf
Merge branch 'develop' into feat/adding-test-for-replying-to-a-comment
JohnAllenTech Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions e2e/articles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@ test.describe("Unauthenticated Articles Page", () => {
);
});

test("Should be able to navigate directly to an article", async ({
page,
}) => {
await page.goto("http://localhost:3000/articles/e2e-test-slug-eqj0ozor");
await expect(page.getByText("Lorem ipsum dolor sit amet,")).toBeVisible();
await expect(
page.getByRole("heading", { name: "Test Article" }),
).toBeVisible();
await expect(
page.getByRole("heading", { name: "Written by E2E Test User One" }),
).toBeVisible();
await expect(page.getByLabel("like-trigger")).toBeVisible();
await expect(page.getByLabel("bookmark-trigger")).toBeVisible();
});

test("Should show bookmark article icon", async ({ page }) => {
await page.goto("http://localhost:3000/articles");

Expand Down Expand Up @@ -274,4 +289,46 @@ test.describe("Authenticated Articles Page", () => {

await expect(page.getByText(commentContent)).toBeVisible();
});

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();
await expect(page.getByText("Lorem ipsum dolor sit amet,")).toBeVisible();
await expect(
page.getByRole("heading", { name: "Test Article" }),
).toBeVisible();
await expect(
page.getByRole("heading", { name: "Written by E2E Test User One" }),
).toBeVisible();
await expect(
page.getByRole("heading", { name: "Discussion (0)" }),
).toBeVisible();
await expect(page.getByLabel("like-trigger")).toBeVisible();
await expect(page.getByLabel("bookmark-trigger")).toBeVisible();

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);

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),
).toBeVisible();
});
});
65 changes: 65 additions & 0 deletions e2e/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import dotenv from "dotenv";
import postgres from "postgres";
import { drizzle } from "drizzle-orm/postgres-js";
import { post, comment } from "@/server/db/schema";

dotenv.config(); // Load .env file contents into process.env

export const setup = async () => {
if (
!process.env.DATABASE_URL ||
!process.env.E2E_USER_ONE_ID ||
!process.env.E2E_USER_TWO_ID
) {
throw new Error("Missing env variables for DB clean up script");
}
Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling with specific missing variable information.

The current error message doesn't indicate which specific environment variables are missing, making debugging more difficult.

Consider implementing more detailed error reporting:

-    throw new Error("Missing env variables for DB clean up script");
+    const missingVars = [
+      ['DATABASE_URL', process.env.DATABASE_URL],
+      ['E2E_USER_ONE_ID', process.env.E2E_USER_ONE_ID],
+      ['E2E_USER_TWO_ID', process.env.E2E_USER_TWO_ID],
+    ].filter(([name, value]) => !value).map(([name]) => name);
+    throw new Error(`Missing required environment variables: ${missingVars.join(', ')}`);
📝 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.

Suggested change
export const setup = async () => {
if (
!process.env.DATABASE_URL ||
!process.env.E2E_USER_ONE_ID ||
!process.env.E2E_USER_TWO_ID
) {
throw new Error("Missing env variables for DB clean up script");
}
export const setup = async () => {
if (
!process.env.DATABASE_URL ||
!process.env.E2E_USER_ONE_ID ||
!process.env.E2E_USER_TWO_ID
) {
const missingVars = [
['DATABASE_URL', process.env.DATABASE_URL],
['E2E_USER_ONE_ID', process.env.E2E_USER_ONE_ID],
['E2E_USER_TWO_ID', process.env.E2E_USER_TWO_ID],
].filter(([name, value]) => !value).map(([name]) => name);
throw new Error(`Missing required environment variables: ${missingVars.join(', ')}`);
}

const db = drizzle(postgres(process.env.DATABASE_URL as string));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add database connection error handling and cleanup.

The database connection should be properly handled with error catching and cleanup mechanisms.

Consider implementing connection handling:

-  const db = drizzle(postgres(process.env.DATABASE_URL as string));
+  let sql;
+  try {
+    sql = postgres(process.env.DATABASE_URL as string);
+    const db = drizzle(sql);
+    return db;
+  } catch (error) {
+    console.error('Failed to establish database connection:', error);
+    throw error;
+  } finally {
+    // Ensure to add cleanup in the main try-catch block
+    // await sql?.end();
+  }

Committable suggestion was skipped due to low confidence.


const addE2EArticleAndComment = async (
authorId: string,
commenterId: string,
) => {
const postId = "1nFnMmN5";
const now = new Date().toISOString();
await db
.insert(post)
.values({
id: postId,
published: now,
excerpt: "Lorem ipsum dolor sit amet",
updatedAt: now,
slug: "e2e-test-slug-eqj0ozor",
likes: 10,
readTimeMins: 3,
title: "Test Article",
body: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas vitae ipsum id metus vestibulum rutrum eget a diam. Integer eget vulputate risus, ac convallis nulla. Mauris sed augue nunc. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Nam congue posuere tempor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Ut ac augue non libero ullamcorper ornare. Ut commodo ligula vitae malesuada maximus. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Etiam sagittis justo non justo placerat, a dapibus sapien volutpat. Nullam ullamcorper sodales justo sed.",
userId: authorId,
})
.onConflictDoNothing()
.returning();

await db
.insert(comment)
.values({
postId,
body: "What a great article! Thanks for sharing",
userId: commenterId,
})
.onConflictDoNothing()
.returning();
};
Comment on lines +18 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve test data management and validation.

Several improvements could make the test data setup more robust:

  1. The hardcoded postId could cause conflicts in parallel test runs
  2. No validation that the insertions were successful
  3. The article body content is unnecessarily long for a test

Consider these improvements:

   const addE2EArticleAndComment = async (
     authorId: string,
     commenterId: string,
   ) => {
-    const postId = "1nFnMmN5";
+    const postId = `e2e-test-${Date.now()}-${Math.random().toString(36).slice(2)}`;
     const now = new Date().toISOString();
-    await db
+    const [insertedPost] = await db
       .insert(post)
       .values({
         // ... other fields ...
-        body: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas vitae ipsum id metus vestibulum rutrum eget a diam. Integer eget vulputate risus, ac convallis nulla. Mauris sed augue nunc. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Nam congue posuere tempor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Ut ac augue non libero ullamcorper ornare. Ut commodo ligula vitae malesuada maximus. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Etiam sagittis justo non justo placerat, a dapibus sapien volutpat. Nullam ullamcorper sodales justo sed.",
+        body: "This is a test article content for E2E testing.",
         userId: authorId,
       })
       .onConflictDoNothing()
       .returning();

+    if (!insertedPost) {
+      throw new Error(`Failed to insert test article with ID ${postId}`);
+    }

-    await db
+    const [insertedComment] = await db
       .insert(comment)
       // ... rest of the comment insertion ...
       .returning();

+    if (!insertedComment) {
+      throw new Error(`Failed to insert test comment for article ${postId}`);
+    }
+
+    return { postId, commentId: insertedComment.id };
   };
📝 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.

Suggested change
const addE2EArticleAndComment = async (
authorId: string,
commenterId: string,
) => {
const postId = "1nFnMmN5";
const now = new Date().toISOString();
await db
.insert(post)
.values({
id: postId,
published: now,
excerpt: "Lorem ipsum dolor sit amet",
updatedAt: now,
slug: "e2e-test-slug-eqj0ozor",
likes: 10,
readTimeMins: 3,
title: "Test Article",
body: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas vitae ipsum id metus vestibulum rutrum eget a diam. Integer eget vulputate risus, ac convallis nulla. Mauris sed augue nunc. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Nam congue posuere tempor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Ut ac augue non libero ullamcorper ornare. Ut commodo ligula vitae malesuada maximus. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Etiam sagittis justo non justo placerat, a dapibus sapien volutpat. Nullam ullamcorper sodales justo sed.",
userId: authorId,
})
.onConflictDoNothing()
.returning();
await db
.insert(comment)
.values({
postId,
body: "What a great article! Thanks for sharing",
userId: commenterId,
})
.onConflictDoNothing()
.returning();
};
const addE2EArticleAndComment = async (
authorId: string,
commenterId: string,
) => {
const postId = `e2e-test-${Date.now()}-${Math.random().toString(36).slice(2)}`;
const now = new Date().toISOString();
const [insertedPost] = await db
.insert(post)
.values({
id: postId,
published: now,
excerpt: "Lorem ipsum dolor sit amet",
updatedAt: now,
slug: "e2e-test-slug-eqj0ozor",
likes: 10,
readTimeMins: 3,
title: "Test Article",
body: "This is a test article content for E2E testing.",
userId: authorId,
})
.onConflictDoNothing()
.returning();
if (!insertedPost) {
throw new Error(`Failed to insert test article with ID ${postId}`);
}
const [insertedComment] = await db
.insert(comment)
.values({
postId,
body: "What a great article! Thanks for sharing",
userId: commenterId,
})
.onConflictDoNothing()
.returning();
if (!insertedComment) {
throw new Error(`Failed to insert test comment for article ${postId}`);
}
return { postId, commentId: insertedComment.id };
};


try {
console.log("creating articles");

await addE2EArticleAndComment(
process.env.E2E_USER_ONE_ID as string,
process.env.E2E_USER_TWO_ID as string,
);
console.log("DB setup successful");
} catch (err) {
console.log("Error while setting up DB before E2E test run", err);
}
Comment on lines +52 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and logging in the main execution block.

The current error handling and logging could be more informative for debugging test setup issues.

Consider enhancing the error handling:

   try {
-    console.log("creating articles");
+    console.log("[E2E Setup] Starting database initialization...");

     await addE2EArticleAndComment(
       process.env.E2E_USER_ONE_ID as string,
       process.env.E2E_USER_TWO_ID as string,
     );
-    console.log("DB setup successful");
+    console.log("[E2E Setup] Successfully initialized test data");
   } catch (err) {
-    console.log("Error while setting up DB before E2E test run", err);
+    console.error("[E2E Setup] Failed to initialize test data:", err);
+    throw err; // Re-throw to ensure test setup fails properly
   }
📝 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.

Suggested change
try {
console.log("creating articles");
await addE2EArticleAndComment(
process.env.E2E_USER_ONE_ID as string,
process.env.E2E_USER_TWO_ID as string,
);
console.log("DB setup successful");
} catch (err) {
console.log("Error while setting up DB before E2E test run", err);
}
try {
console.log("[E2E Setup] Starting database initialization...");
await addE2EArticleAndComment(
process.env.E2E_USER_ONE_ID as string,
process.env.E2E_USER_TWO_ID as string,
);
console.log("[E2E Setup] Successfully initialized test data");
} catch (err) {
console.error("[E2E Setup] Failed to initialize test data:", err);
throw err; // Re-throw to ensure test setup fails properly
}

};

export default setup;
1 change: 1 addition & 0 deletions playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { defineConfig, devices } from "@playwright/test";
* See https://playwright.dev/docs/test-configuration.
*/
export default defineConfig({
globalSetup: "./e2e/setup.ts",
globalTeardown: "./e2e/teardown.ts",
testDir: "e2e",
/* Run tests in files in parallel */
Expand Down
Loading