Skip to content

chore: moving out user auth out into a util for E2E tests #1181

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 27, 2024
Merged

chore: moving out user auth out into a util for E2E tests #1181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02aabfe
chore: moving out from user auth out into a util
JohnAllenTech Oct 26, 2024
984b47d
chore: missed some updates in the README
JohnAllenTech Oct 26, 2024
ec6021d
Merge branch 'develop' into feat/prep-for-multiple-user-e2e-tests
NiallJoeMaher Oct 26, 2024
d11dd23
chore: adding logging to better understand why user setup is failing …
JohnAllenTech Oct 27, 2024
b78ae71
chore: removing logs
JohnAllenTech Oct 27, 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
8 changes: 4 additions & 4 deletions e2e/articles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import { test, expect } from "playwright/test";
import { randomUUID } from "crypto";
import { loggedInAsUserOne } from "./utils";

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(
Expand Down Expand Up @@ -133,6 +130,9 @@ test.describe("Unauthenticated Articles Page", () => {
});

test.describe("Authenticated Articles Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
test("Should show recent bookmarks", async ({ page, isMobile }) => {
await page.goto("http://localhost:3000/articles");
await expect(
Expand Down
54 changes: 0 additions & 54 deletions e2e/auth.setup.ts
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions e2e/home.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { test, expect } from "@playwright/test";
import { loggedInAsUserOne } from "./utils";

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

Expand All @@ -24,9 +28,6 @@ test.describe("Authenticated homepage", () => {
});

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

Expand Down
4 changes: 4 additions & 0 deletions e2e/login.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { test, expect } from "playwright/test";
import "dotenv/config";
import { loggedInAsUserOne } from "./utils";

test.describe("Unauthenticated Login Page", () => {
test.beforeEach(async ({ page }) => {
Expand Down Expand Up @@ -31,6 +32,9 @@ test.describe("Unauthenticated Login Page", () => {
});

test.describe("Authenticated Login Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(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");
Expand Down
7 changes: 4 additions & 3 deletions e2e/my-posts.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import test from "@playwright/test";
import { loggedInAsUserOne } from "./utils";

test.describe("Unauthenticated my-posts Page", () => {
test.beforeEach(async ({ page }) => {
await page.context().clearCookies();
});
//
// Replace with tests for unauthenticated users
});

test.describe("Authenticated my-posts Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
//
// Replace with tests for authenticated users
});
7 changes: 4 additions & 3 deletions e2e/settings.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import test from "@playwright/test";
import { loggedInAsUserOne } from "./utils";

test.describe("Unauthenticated setttings Page", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in test description

There's a typo in "setttings" (three t's) in the test suite description.

-test.describe("Unauthenticated setttings Page", () => {
+test.describe("Unauthenticated settings Page", () => {
📝 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.describe("Unauthenticated setttings Page", () => {
test.describe("Unauthenticated settings Page", () => {

test.beforeEach(async ({ page }) => {
await page.context().clearCookies();
});
//
// Replace with tests for unauthenticated users
});

test.describe("Authenticated settings Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
//
// Replace with tests for authenticated users
});
Comment on lines 1 to 15
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding test organization comments

To improve maintainability and clarity, consider adding JSDoc comments for each test suite to document the test organization and coverage goals.

 import test from "@playwright/test";
 import { loggedInAsUserOne } from "./utils";
 
+/**
+ * Test suite for unauthenticated user interactions with the settings page.
+ * Verifies proper handling of unauthorized access and redirects.
+ */
 test.describe("Unauthenticated settings Page", () => {
   // TODO: Implement tests for unauthenticated scenarios
 });
 
+/**
+ * Test suite for authenticated user interactions with the settings page.
+ * Verifies settings management functionality for logged-in users.
+ */
 test.describe("Authenticated settings Page", () => {
   test.beforeEach(async ({ page }) => {
     await loggedInAsUserOne(page);
   });
   // TODO: Implement tests for authenticated scenarios
 });
📝 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
import test from "@playwright/test";
import { loggedInAsUserOne } from "./utils";
test.describe("Unauthenticated setttings Page", () => {
test.beforeEach(async ({ page }) => {
await page.context().clearCookies();
});
//
// Replace with tests for unauthenticated users
});
test.describe("Authenticated settings Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
//
// Replace with tests for authenticated users
});
import test from "@playwright/test";
import { loggedInAsUserOne } from "./utils";
/**
* Test suite for unauthenticated user interactions with the settings page.
* Verifies proper handling of unauthorized access and redirects.
*/
test.describe("Unauthenticated settings Page", () => {
//
// Replace with tests for unauthenticated users
});
/**
* Test suite for authenticated user interactions with the settings page.
* Verifies settings management functionality for logged-in users.
*/
test.describe("Authenticated settings Page", () => {
test.beforeEach(async ({ page }) => {
await loggedInAsUserOne(page);
});
//
// Replace with tests for authenticated users
});

1 change: 1 addition & 0 deletions e2e/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./utils";
27 changes: 27 additions & 0 deletions e2e/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { expect, Page } from "@playwright/test";

export const loggedInAsUserOne = async (page: Page) => {
try {
expect(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety for environment variable.

Consider adding runtime type checking for the environment variable value, not just its existence.

-    expect(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined();
+    const sessionId = process.env.E2E_USER_ONE_SESSION_ID;
+    expect(sessionId).toBeDefined();
+    expect(typeof sessionId === 'string' && sessionId.length > 0).toBeTruthy();
📝 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(process.env.E2E_USER_ONE_SESSION_ID).toBeDefined();
const sessionId = process.env.E2E_USER_ONE_SESSION_ID;
expect(sessionId).toBeDefined();
expect(typeof sessionId === 'string' && sessionId.length > 0).toBeTruthy();


await page.context().addCookies([
{
name: "next-auth.session-token",
value: process.env.E2E_USER_ONE_SESSION_ID as string,
domain: "localhost",
path: "/",
sameSite: "Lax",
},
]);
Comment on lines +7 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance cookie security configuration.

The cookie configuration is missing important security flags:

  1. httpOnly to prevent XSS attacks
  2. secure flag for HTTPS-only transmission

Apply this diff to improve security:

     await page.context().addCookies([
       {
         name: "next-auth.session-token",
         value: process.env.E2E_USER_ONE_SESSION_ID as string,
         domain: "localhost",
         path: "/",
         sameSite: "Lax",
+        httpOnly: true,
+        secure: process.env.NODE_ENV === "production"
       },
     ]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await page.context().addCookies([
{
name: "next-auth.session-token",
value: process.env.E2E_USER_ONE_SESSION_ID as string,
domain: "localhost",
path: "/",
sameSite: "Lax",
},
]);
await page.context().addCookies([
{
name: "next-auth.session-token",
value: process.env.E2E_USER_ONE_SESSION_ID as string,
domain: "localhost",
path: "/",
sameSite: "Lax",
httpOnly: true,
secure: process.env.NODE_ENV === "production"
},
]);


expect(
(await page.context().cookies()).find(
(cookie) => cookie.name === "next-auth.session-token",
),
).toBeTruthy();
Comment on lines +17 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Strengthen cookie verification.

The current verification only checks for cookie existence. Consider validating the cookie value matches what was set.

     expect(
-      (await page.context().cookies()).find(
-        (cookie) => cookie.name === "next-auth.session-token",
-      ),
-    ).toBeTruthy();
+      (await page.context().cookies()).find(
+        (cookie) => cookie.name === "next-auth.session-token" && 
+                    cookie.value === sessionId
+      ),
+    ).toBeTruthy("Session cookie was not set correctly");

Committable suggestion was skipped due to low confidence.

} catch (err) {
// console.log(err);

throw Error("Error while authenticating E2E test user one");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling and logging.

The error handling could be enhanced:

  1. Remove commented-out code
  2. Include the original error details in the thrown error
  3. Consider adding structured logging for debugging
-    // console.log(err);
-
-    throw Error("Error while authenticating E2E test user one");
+    throw new Error(
+      `Error while authenticating E2E test user one: ${err instanceof Error ? err.message : String(err)}`
+    );
📝 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
// console.log(err);
throw Error("Error while authenticating E2E test user one");
}
throw new Error(
`Error while authenticating E2E test user one: ${err instanceof Error ? err.message : String(err)}`
);

};
10 changes: 0 additions & 10 deletions playwright.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,36 +39,26 @@ export default defineConfig({
{ name: "setup", testMatch: /auth.setup\.ts/ },
{
name: "Desktop Chrome",
use: {
storageState: "playwright/.auth/browser.json",
},
dependencies: ["setup"],
},

// Example other browsers
{
name: "Desktop Firefox",
use: {
...devices["Desktop Firefox"],
storageState: "playwright/.auth/browser.json",
},
dependencies: ["setup"],
},
{
name: "Mobile Chrome",
use: {
...devices["Pixel 9"],
storageState: "playwright/.auth/browser.json",
},
dependencies: ["setup"],
},
{
name: "Mobile Safari",
use: {
...devices["iPhone 16"],
storageState: "playwright/.auth/browser.json",
},
dependencies: ["setup"],
},
],

Expand Down
15 changes: 0 additions & 15 deletions playwright/.auth/browser.json
View file
Open in desktop

This file was deleted.

Loading