Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Feat/improving existing e2e tests#1125

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
NiallJoeMaher merged 3 commits into from
Oct 15, 2024
Merged

Feat/improving existing e2e tests #1125

Show file tree
Hide file tree
Changes from all 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 })=>{awaitpage.goto("/");// Navigate to the homepageconstimagesWithoutAltText=awaitpage.$$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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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);

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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", () => {

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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 logicawaitpage.goto("http://localhost:3000/login");awaitpage.fill('input[name="email"]','user@example.com');awaitpage.fill('input[name="password"]','password');awaitpage.click('button[type="submit"]');awaitpage.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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeVisible();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeVisible();
awaitexpect(page.getByText("CodúBeta Sign in or create")).toBeVisible();
awaitexpect(
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/");

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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/");
awaitexpect(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

@coderabbitaicoderabbitaiBotOct 15, 2024

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

@coderabbitaicoderabbitaiBotOct 15, 2024

Copy link
Copy Markdown
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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
awaitpage.goto("http://localhost:3000/get-started");
expect(page.url()).toEqual("http://localhost:3000/");
awaitexpect(page.getByText("CodúBetaSign in or create")).toBeHidden();
awaitexpect(
page.getByRole("heading",{name: "Sign in or create your account"}),
).toBeHidden();
awaitexpect(page.getByRole("link",{name: "return home"})).toBeHidden();
if(!isMobile){
awaitexpect(
page.getByRole("button",{name: "Sign up for free"}),
).toBeHidden();
awaitexpect(
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();
});
});