Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/e2e-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ jobs:
NEXTAUTH_SECRET: ${{ secrets.NEXTAUTH_SECRET }}
E2E_USER_EMAIL: e2e@codu.co
E2E_USER_ID: 8e3179ce-f32b-4d0a-ba3b-234d66b836ad
E2E_USER_SESSION_ID: df8a11f2-f20a-43d6-80a0-a213f1efedc1
E2E_USER_ONE_SESSION_ID: df8a11f2-f20a-43d6-80a0-a213f1efedc1

steps:
- name: Checkout repository
Expand Down
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ You shouldn't need to change the default value here. This is a variable used by
NEXTAUTH_URL=http://localhost:3000/api/auth
```

### E2E_USER_SESSION_ID
### E2E_USER_ONE_SESSION_ID

This is the sessionToken uuid that .
This is the sessionToken uuid that is used to identify a users current active session.
This is currently hardcoded and there is no reason to change this until we require multiple E2E test users within the same test suite

### E2E_USER_ID
Expand Down Expand Up @@ -173,7 +173,7 @@ Please ensure you have the following variables set in your `.env` file:

- `E2E_USER_ID`: The id of the E2E user for testing.
- `E2E_USER_EMAIL`: The email of the E2E user for testing.
- `E2E_USER_SESSION_ID`: The session id that the user will use to authenticate.
- `E2E_USER_ONE_SESSION_ID`: The session id that the user will use to authenticate.


Note the sample .env [here](./sample.env) is fine to use.
Expand Down
2 changes: 1 addition & 1 deletion drizzle/seed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import postgres from "postgres";
const DATABASE_URL = process.env.DATABASE_URL || "";
// These can be removed in a follow on PR. Until this hits main we cant add E2E_USER_* stuff to the env.
const E2E_SESSION_ID =
process.env.E2E_USER_SESSION_ID || "df8a11f2-f20a-43d6-80a0-a213f1efedc1";
process.env.E2E_USER_ONE_SESSION_ID || "df8a11f2-f20a-43d6-80a0-a213f1efedc1";
const E2E_USER_ID =
process.env.E2E_USER_ID || "8e3179ce-f32b-4d0a-ba3b-234d66b836ad";
const E2E_USER_EMAIL = process.env.E2E_USER_EMAIL || "e2e@codu.co";
Expand Down
8 changes: 4 additions & 4 deletions e2e/articles.spec.ts
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

This file was deleted.

7 changes: 4 additions & 3 deletions e2e/home.spec.ts
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
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
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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./utils";
25 changes: 25 additions & 0 deletions e2e/utils/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
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) {
throw Error("Error while authenticating E2E test user one");
}
};
10 changes: 0 additions & 10 deletions playwright.config.ts
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

This file was deleted.

2 changes: 1 addition & 1 deletion sample.env
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ DATABASE_URL=postgresql://postgres:secret@127.0.0.1:5432/postgres

E2E_USER_EMAIL=e2e@codu.co
E2E_USER_ID=8e3179ce-f32b-4d0a-ba3b-234d66b836ad
E2E_USER_SESSION_ID=df8a11f2-f20a-43d6-80a0-a213f1efedc1
E2E_USER_ONE_SESSION_ID=df8a11f2-f20a-43d6-80a0-a213f1efedc1
Loading