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.

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

Jump to bottom

Feat/improving existing e2e tests #1125

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ee1736
chore: improved current E2E tests and enhanced labels
JohnAllenTech Oct 15, 2024
8105aa8
chore: added more tests around article page and unauth/auth views
JohnAllenTech Oct 15, 2024
ce889bf
Merge branch 'develop' into feat/improving-existing-e2e-tests
NiallJoeMaher Oct 15, 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
13 changes: 13 additions & 0 deletions e2e/accessibility.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { test, expect } from "@playwright/test";

test.describe("Accessibility Tests", () => {
test.describe("Confirm all images on homepage have alt text", () => {
test("Shared content", async ({ page }) => {
const imagesWithoutAltText = await page.$$eval(
"img:not([alt])",
(images) => images.length,
);
expect(imagesWithoutAltText).toBe(0); // All images should have alt text
});
Comment on lines +5 to +11
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

Consider enhancing the test case implementation.

The test case effectively checks for alt text on images, which is crucial for accessibility. However, there are a couple of points to consider:

  1. The test doesn't navigate to the homepage before performing the check. Consider adding a page.goto() call at the beginning of the test.

  2. The test title "Shared content" might not be the most descriptive for this specific check. A more explicit title like "All images should have alt text" would better describe the test's purpose.

Here's a suggested improvement:

test("All images should have alt text", async ({ page }) => {
  await page.goto("/"); // Navigate to the homepage
  const imagesWithoutAltText = await page.$$eval(
    "img:not([alt])",
    (images) => images.length,
  );
  expect(imagesWithoutAltText).toBe(0);
});

This change ensures the test is performed on the homepage and provides a more descriptive test title.

});
});
95 changes: 94 additions & 1 deletion e2e/articles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,99 @@
import { test, expect } from "playwright/test";

test.describe("Articles", () => {
test.describe("Unauthenticated Articles Page", () => {
test.beforeEach(async ({ page }) => {
await page.context().clearCookies();
});

test("Should show popular tags", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/articles");
await expect(
page.getByRole("heading", { name: "Popular topics" }),
).toBeVisible({ visible: !isMobile });
Comment on lines +11 to +12
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

Correct improper usage of toBeVisible() method

The toBeVisible() assertion does not accept a visible option parameter. To conditionally check visibility based on isMobile, you should use .toBeVisible() or .not.toBeVisible() accordingly.

Apply this diff to fix the assertions:

// For lines 11-12
-        ).toBeVisible({ visible: !isMobile });
+        if (isMobile) {
+          await expect(
+            page.getByRole("heading", { name: "Popular topics" }),
+          ).not.toBeVisible();
+        } else {
+          await expect(
+            page.getByRole("heading", { name: "Popular topics" }),
+          ).toBeVisible();
+        }

// For lines 15-16
-        ).toBeVisible({ visible: !isMobile });
+        if (isMobile) {
+          await expect(
+            page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+          ).not.toBeVisible();
+        } else {
+          await expect(
+            page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+          ).toBeVisible();
+        }

// For lines 67-68
-        ).toBeVisible({ visible: !isMobile });
+        if (isMobile) {
+          await expect(
+            page.getByRole("heading", { name: "Popular topics" }),
+          ).not.toBeVisible();
+        } else {
+          await expect(
+            page.getByRole("heading", { name: "Popular topics" }),
+          ).toBeVisible();
+        }

// For lines 71-72
-        ).toBeVisible({ visible: !isMobile });
+        if (isMobile) {
+          await expect(
+            page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+          ).not.toBeVisible();
+        } else {
+          await expect(
+            page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
+          ).toBeVisible();
+        }

Also applies to: 15-16, 67-68, 71-72


await expect(
page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
).toBeVisible({ visible: !isMobile });
});

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

await expect(
page.getByRole("heading", { name: "Recent bookmarks" }),
).toBeHidden();

await expect(
page.locator("article").first().getByLabel("Bookmark this post"),
).toBeHidden();
});
test("Should load more articles when scrolling to the end of the page", async ({
page,
isMobile,
}) => {
await page.goto("http://localhost:3000/articles");
// Waits for articles to be loaded
await page.waitForSelector("article");

const initialArticleCount = await page.$$eval(
"article",
(articles) => articles.length,
);

if (!isMobile) {
await page.getByText("Code Of Conduct").scrollIntoViewIfNeeded();
await page.waitForTimeout(5000);
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

Avoid using fixed delays with waitForTimeout; use proper waiting mechanisms

Using await page.waitForTimeout(5000); introduces fixed delays which can slow down tests and may not be reliable. Consider using more robust waiting methods like waiting for network idle or waiting for specific elements to appear.

Apply this diff to improve the test:

-      await page.waitForTimeout(5000);
+      await page.waitForLoadState('networkidle');

Alternatively, wait for a specific element that appears after loading more articles:

-      await page.waitForTimeout(5000);
+      await page.waitForSelector("article:nth-child(${initialArticleCount + 1})");

Committable suggestion was skipped due to low confidence.

const finalArticleCount = await page.$$eval(
"article",
(articles) => articles.length,
);
expect(finalArticleCount).toBeGreaterThan(initialArticleCount);
}

await expect(page.getByText("Home")).toBeVisible();
await expect(
page.getByLabel("Footer").getByRole("link", { name: "Events" }),
).toBeVisible();
await expect(page.getByText("Sponsorship")).toBeVisible();
await expect(page.getByText("Code Of Conduct")).toBeVisible();
});
});

test.describe("Authenticated Articles Page", () => {
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

Set up authentication context in authenticated tests

The authenticated tests need to ensure that the user is authenticated before running the tests. Currently, there is no beforeEach hook to set up the authentication context.

Consider adding a beforeEach hook to authenticate the user before each test:

test.beforeEach(async ({ page }) => {
  // Replace with your authentication logic
  await page.goto("http://localhost:3000/login");
  await page.fill('input[name="email"]', 'user@example.com');
  await page.fill('input[name="password"]', 'password');
  await page.click('button[type="submit"]');
  await page.waitForNavigation();
});

If you have a helper function or fixture for authentication, you can use that instead to keep your tests DRY.

test("Should show recent bookmarks", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/articles");
await expect(
page.getByRole("heading", { name: "Popular topics" }),
).toBeVisible({ visible: !isMobile });

await expect(
page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
).toBeVisible({ visible: !isMobile });

await expect(
page.getByRole("heading", { name: "Recent bookmarks" }),
).toBeVisible({ visible: !isMobile });
});

test("Should show bookmark article icon", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/articles");
await expect(
page.getByRole("heading", { name: "Popular topics" }),
).toBeVisible({ visible: !isMobile });

await expect(
page.getByRole("link", { name: '"Codú Writing Challenge" text' }),
).toBeVisible({ visible: !isMobile });

await expect(
page.getByRole("heading", { name: "Recent bookmarks" }),
).toBeVisible({ visible: !isMobile });

await expect(
page.locator("article").first().getByLabel("Bookmark this post"),
).toBeVisible();
});

test("Should load more articles when scrolling to the end of the page", async ({
page,
isMobile,
Expand Down
6 changes: 2 additions & 4 deletions e2e/auth.setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ setup("authenticate", async ({ page }) => {
}

try {
//expect(process.env.E2E_USER_SESSION_ID).toBeDefined(); removing until I can get it all working.

const E2E_USER_SESSION_ID = "df8a11f2-f20a-43d6-80a0-a213f1efedc1";
expect(process.env.E2E_USER_SESSION_ID).toBeDefined();

await page.context().addCookies([
{
name: "next-auth.session-token",
value: E2E_USER_SESSION_ID as string,
value: process.env.E2E_USER_SESSION_ID as string,
domain: "localhost",
path: "/",
sameSite: "Lax",
Expand Down
46 changes: 17 additions & 29 deletions e2e/home.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,33 @@
import { test, expect } from "@playwright/test";

test.describe("Testing homepage views", () => {
test("Authenticated homepage view", async ({ page, isMobile }) => {
test.describe("Authenticated homepage", () => {
test("Homepage view", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/");

await expect(page.locator("h1")).not.toContainText("Unwanted text");

if (!isMobile)
const elementVisible = await page
.locator('text="Popular topics"')
.isVisible();

if (isMobile) {
expect(elementVisible).toBe(false);
} else {
await expect(
page.getByRole("link", {
name: "Your Posts",
}),
).toBeVisible();
expect(elementVisible).toBe(true);
}
});
test("Unauthenticated homepage view", async ({ page }) => {
});

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

await expect(page.locator("h1")).not.toContainText("Unwanted text");
Expand All @@ -25,29 +38,4 @@ test.describe("Testing homepage views", () => {
"The free web developer community",
);
});

test("Authenticated landing page view", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/");

const elementVisible = await page
.locator('text="Popular topics"')
.isVisible();

if (isMobile) {
expect(elementVisible).toBe(false);
} else {
expect(elementVisible).toBe(true);
}
});

test.describe("Confirm image accessibiliy content", () => {
test("Shared content", async ({ page }) => {
// Accessibility
const imagesWithoutAltText = await page.$$eval(
"img:not([alt])",
(images) => images.length,
);
expect(imagesWithoutAltText).toBe(0); // All images should have alt text
});
});
});
60 changes: 48 additions & 12 deletions e2e/login.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,59 @@
import { test, expect } from "playwright/test";
import "dotenv/config";

test.describe("Login Page", () => {
test("should display the welcome message", async ({ page }) => {
await page.goto("http://localhost:3000/get-started");
const welcomeMessage = page.getByText("Sign in or create your accounttton");
expect(welcomeMessage).toBeTruthy();
});
test("should display the Github login button", async ({ page }) => {
test.describe("Unauthenticated Login Page", () => {
test.beforeEach(async ({ page }) => {
await page.context().clearCookies();
await page.goto("http://localhost:3000/get-started");
await page.waitForTimeout(3000);
});
test("Sign up page contains sign up links", async ({ page, isMobile }) => {
await expect(page.getByText("CodúBetaSign in or create")).toBeVisible();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeVisible();
Comment on lines +10 to +13
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

Correct the expected text in the assertion

The text "CodúBetaSign in or create" seems to be missing a space between "CodúBeta" and "Sign in or create". This may cause the test to fail if the actual text on the page includes a space. Please verify the expected text for accuracy.

Consider updating the assertion:

-await expect(page.getByText("CodúBetaSign in or create")).toBeVisible();
+await expect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
📝 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
await expect(page.getByText("CodúBetaSign in or create")).toBeVisible();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeVisible();
await expect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeVisible();

await expect(page.getByRole("link", { name: "return home" })).toBeVisible();
if (!isMobile) {
await expect(
page.getByRole("button", { name: "Sign up for free" }),
).toBeVisible();
await expect(
page.getByRole("button", { name: "Sign in", exact: true }),
).toBeVisible();
}
});
test("Login page contains GitHub button", async ({ page }) => {
await expect(page.getByTestId("github-login-button")).toBeVisible();
});

test("should display the Gitlab login button", async ({ page }) => {
await page.context().clearCookies();
await page.goto("http://localhost:3000/get-started");
await page.waitForLoadState();
test("Login page contains GitLab button", async ({ page }) => {
await expect(page.getByTestId("gitlab-login-button")).toBeVisible();
});
});

test.describe("Authenticated Login Page", () => {
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/");
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

Await the URL assertion to ensure navigation completion

The assertion expect(page.url()).toEqual("http://localhost:3000/"); should be awaited to ensure that the URL has fully updated before the assertion runs. Without await, the test might pass prematurely.

Update the code to use await with toHaveURL for better reliability:

-expect(page.url()).toEqual("http://localhost:3000/");
+await expect(page).toHaveURL("http://localhost:3000/");
📝 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
expect(page.url()).toEqual("http://localhost:3000/");
await expect(page).toHaveURL("http://localhost:3000/");

await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeHidden();
Comment on lines +38 to +41
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

Correct the expected text in the assertion

Similarly, in the authenticated tests, the text "CodúBetaSign in or create" may be missing a space. Ensure the expected text matches the actual on-page text to prevent false negatives in your tests.

Update the assertion:

-await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
+await expect(page.getByText("CodúBeta Sign in or create")).toBeHidden();
📝 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
await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeHidden();
await expect(page.getByText("CodúBeta Sign in or create")).toBeHidden();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeHidden();

await expect(page.getByRole("link", { name: "return home" })).toBeHidden();
if (!isMobile) {
await expect(
page.getByRole("button", { name: "Sign up for free" }),
).toBeHidden();
await expect(
page.getByRole("button", { name: "Sign in", exact: true }),
).toBeHidden();
}
});
Comment on lines +34 to +51
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

Rename the test to reflect its purpose

The test "Sign up page contains sign up links" under the "Authenticated Login Page" suite is checking that certain elements are hidden and that authenticated users are redirected. The test name might be misleading.

Consider renaming the test for clarity:

-test("Sign up page contains sign up links", async ({ page, isMobile }) => {
+test("Authenticated users are redirected from get-started page", async ({ page, isMobile }) => {
📝 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
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/");
await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeHidden();
await expect(page.getByRole("link", { name: "return home" })).toBeHidden();
if (!isMobile) {
await expect(
page.getByRole("button", { name: "Sign up for free" }),
).toBeHidden();
await expect(
page.getByRole("button", { name: "Sign in", exact: true }),
).toBeHidden();
}
});
test("Authenticated users are redirected from get-started page", 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/");
await expect(page.getByText("CodúBetaSign in or create")).toBeHidden();
await expect(
page.getByRole("heading", { name: "Sign in or create your account" }),
).toBeHidden();
await expect(page.getByRole("link", { name: "return home" })).toBeHidden();
if (!isMobile) {
await expect(
page.getByRole("button", { name: "Sign up for free" }),
).toBeHidden();
await expect(
page.getByRole("button", { name: "Sign in", exact: true }),
).toBeHidden();
}
});

test("Login page contains GitHub button", async ({ page }) => {
await expect(page.getByTestId("github-login-button")).toBeHidden();
});

test("Login page contains GitLab button", async ({ page }) => {
await expect(page.getByTestId("gitlab-login-button")).toBeHidden();
});
});
Loading