Migrate e2e tests to use backend instead of mock data#3591
Migrate e2e tests to use backend instead of mock data#3591ahmedxgouda wants to merge 50 commits intoOWASP:mainfrom
Conversation
github-actions
bot
added
docs
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI, docker-compose and frontend build/config were changed to run real frontend E2E: new frontend/e2e images and services, Playwright/Next.js rewrites and env wiring added, many Playwright tests removed in-test GraphQL/CSRF mocks and refactored to navigate the running app. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
frontend/__tests__/e2e/pages/Committees.spec.ts (1)
1-17: Migration appears incomplete — mock data is still in use.The PR objective is to migrate e2e tests from mocked data to real backend/database data, but this file still:
- Imports
mockCommitteeData(line 2)- Intercepts API calls with
page.route('**/idx/', ...)to return mock data (lines 7-15)The only change here is adding a 120000ms timeout to
page.goto. If the intent is to test against the real backend, the route interception and mock import should be removed.,
Suggested changes if migrating to real backend
import { expectBreadCrumbsToBeVisible } from '@e2e/helpers/expects' -import { mockCommitteeData } from '@mockData/mockCommitteeData' import { test, expect } from '@playwright/test' test.describe('Committees Page', () => { test.beforeEach(async ({ page }) => { - await page.route('**/idx/', async (route) => { - await route.fulfill({ - status: 200, - body: JSON.stringify({ - hits: mockCommitteeData.committees, - nbPages: 2, - }), - }) - }) await page.goto('/committees', { timeout: 120000 }) })Note: The test assertions (lines 19-24) will need updating to match actual committee data from the database rather than the mock data values like "Committee 1" and "Sam Stepanyan's avatar".
frontend/__tests__/e2e/pages/CommitteeDetails.spec.ts (1)
8-26: Test assertions use hardcoded mock-like values against a real backend.The assertions reference specific values ("Test Committee", "This is a test committee", "Dec 13, 2024", "Leader 1", "Contributor 1") that appear to be from mock data. If the goal is to use real backend data, these assertions need to either:
- Be updated to match actual database content, or
- Use a seeded dataset that guarantees these exact values
This inconsistency between removing mocks but keeping mock-style assertions may cause test failures.
frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts (1)
8-57: Assertions reference mock data that won't match the real OWASP organization.All test assertions use hardcoded values ("Test Organization", "5K Stars", "1.2K Forks", "Test Issue 1", "User One", etc.) that were designed for mock data. When running against the real backend with
/organizations/owasp, these values will almost certainly not match, causing test failures.Consider:
- Updating assertions to validate actual OWASP organization data, or
- Using more flexible assertions (e.g., checking element existence rather than exact values), or
- Ensuring seed data populates these exact values for the
owasporganizationfrontend/__tests__/e2e/components/Footer.spec.ts (1)
32-34: Inconsistent timeout between desktop and mobile test setup.Desktop tests (line 11) use
{ timeout: 120000 }for navigation, but mobile tests (line 33) use the default timeout. For consistency and reliability, apply the same timeout to mobile tests.Proposed fix
test.beforeEach(async ({ page }) => { - await page.goto('/') + await page.goto('/', { timeout: 120000 }) })frontend/__tests__/e2e/pages/Home.spec.ts (1)
8-121: Live-data assertions are brittle and will likely fail.These tests still expect fixed titles, names, and dates (e.g., “Chapter 1”, “Mar 18, 2025”), which were valid for mocked data but won’t be stable against real DB content. With the backend migration, this makes the suite non-deterministic.
Fix options:
- Seed the DB with known fixtures for e2e and assert those fixtures, or
- Assert on structure/format (e.g., “at least 1 item” per section, regex for date format) and use stable selectors/test-ids.
frontend/__tests__/e2e/pages/Projects.spec.ts (1)
6-16: Remaining/idx/mock conflicts with backend-migration goal.This spec still stubs
**/idx/with mock data, so it’s not exercising the real data path. If the intent is to migrate e2e to live backend data, this should hit the actual service (or a seeded test index) and assert against real results.If
/idx/is intentionally mocked because it’s external, please document that exception in the test or PR description.frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (3)
29-43: Map popup assertion references mock data.Line 41 expects
'OWASP Test Chapter'in the popup, but the test is running against the Rosario chapter. This should be updated to match the actual Rosario chapter popup content.
45-55: Top contributors assertions use placeholder names.The assertions expect
'Contributor 1'and'Contributor 2', which are mock data names. Update these to match the actual top contributors for the Rosario chapter from the database.
65-69: Leaders assertion uses mock data.The assertion expects
'Bob'as the leader name, but this appears to be mock data. Update to match the actual Rosario chapter leader(s) from the database.
🤖 Fix all issues with AI agents
In @.github/workflows/run-ci-cd.yaml:
- Around line 392-398: The docker run command is incorrectly letting the host
shell interpret the && so only the first command executes in-container; update
the Run frontend step to pass a shell to the container (e.g., use sh -c or bash
-c) so the full command sequence "pnpm run build && pnpm run start" runs inside
the container; ensure you keep the detached flag (-d) and the same container
name (--name e2e-nest-frontend) and options (--network host, --env-file, -p
3000:3000) when invoking docker run so the combined command runs in the
container context.
In `@docker/frontend/Dockerfile.e2e.test`:
- Line 18: The Dockerfile's COPY line currently references a non-standard
.pnpmrc; update the COPY statement in docker/frontend/Dockerfile.e2e.test to
copy .npmrc instead of .pnpmrc, and ensure the repository root/frontend contains
a valid .npmrc file (or add one) so pnpm reads the intended configuration;
specifically change the COPY target that includes ".pnpmrc" to ".npmrc" so the
build uses the correct config.
In `@frontend/__tests__/e2e/pages/About.spec.ts`:
- Around line 23-27: The CI fails because CSpell flags "Abhay" as unknown and
the e2e test hardcodes contributor names; fix this by adding "Abhay" to the
project's CSpell dictionary (e.g., add "Abhay" to the "words" array in
cspell.json or equivalent config) and make the test in About.spec.ts less
brittle by avoiding exact-name checks—update the test named "displays
contributor information when data is loaded" (or the calls to
page.getByText('Abhay Mishra', { exact: true })) to assert on a more stable
condition such as the presence of a contributors section, the expected number of
contributors, or matching a name pattern rather than exact hardcoded names so DB
dump changes won't break the test.
- Around line 79-86: The first test "opens user profile in new window when
leader button is clicked" mutates the shared page state and causes the next test
"breadcrumb renders correct segments on /about" to fail; update the
profile-navigation test (or its teardown) to restore the page to /about after
the click (e.g., call page.goBack() or navigate to '/about') or simply move the
breadcrumb test (which uses expectBreadCrumbsToBeVisible) to run before the
profile navigation test so tests don't depend on mutated page state.
- Around line 4-13: The serial About Page suite shares a single Page instance
(variable page) so tests leak state; in the "breadcrumb renders correct segments
on /about" test, restore page state by navigating back to '/about' before
asserting breadcrumbs—i.e., insert await page.goto('/about') at the start of
that test (the one that calls expectBreadCrumbsToBeVisible) to ensure isolation;
also consider renaming the earlier test that mentions "new window" if it doesn't
open a new context.
In `@frontend/__tests__/e2e/pages/CalendarButton.spec.ts`:
- Around line 7-14: The BrowserContext created in test.beforeAll (const context
= await browser.newContext()) is never closed; save the context to an
outer-scope variable (e.g., context) alongside page and then in test.afterAll
call context.close() (instead of or in addition to page.close()) so the context
and its pages are properly disposed; update references in test.beforeAll and
test.afterAll to use that shared context variable.
In `@frontend/__tests__/e2e/pages/ChapterDetails.spec.ts`:
- Around line 23-27: The assertions in the test "should have chapter details
block" check for mock strings that don't match the page navigated to
(/chapters/rosario); update the three expectations in that test to assert the
actual Rosario chapter values (replace 'Location: Test City, Test', 'Region:
Test Region', and 'https://owasp.org/test-chapter' with Rosario's real
city/region/website) or, better, query the test fixture/API for the Rosario
record and use those returned fields so the assertions match runtime data
(locate the test function named should have chapter details block and change the
three expect(...).toBeVisible() calls accordingly).
In `@frontend/__tests__/e2e/pages/CommitteeDetails.spec.ts`:
- Line 5: The test in CommitteeDetails.spec.ts navigates to '/committees/events'
but lacks GraphQL mocking and expects "Test Committee" data—either add a
page.route() GraphQL mock that returns mockCommitteeDetailsData (mirror the
mocking approach used in Committees.spec.ts for idx/) so the committee detail
query returns the expected test payload, or change the test to navigate to a
real committee slug/key and update assertions to match that backend entity;
locate the navigation call (page.goto('/committees/events', { timeout: 120000
})) and the test assertions for "Test Committee" and implement the mocking or
slug change accordingly.
In `@frontend/__tests__/e2e/pages/ProjectDetails.spec.ts`:
- Around line 88-93: The URL assertion in the test 'should have project
repositories' uses
toHaveURL('organizations/OWASP/repositories/www-project-nest') without a leading
slash; update the expectation to the correct canonical path by changing the
argument to '/organizations/OWASP/repositories/www-project-nest' (or
alternatively use a matcher like
toContain('/organizations/OWASP/repositories/www-project-nest') if variability
is expected) in the test function so page.getByText('www-project-nest') click
assertion verifies the proper absolute URL.
In `@frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts`:
- Line 5: The test uses page.goto('organizations/OWASP/repositories/nest') but
asserts hardcoded UI values without any mocked API responses; update the test to
either call the existing mockDashboardCookies helper or add page.route()
handlers to intercept the repository and PR API endpoints and return the
expected mock payloads (containing "Test Repo", "A sample test repository",
dates, star counts, and PR titles) so assertions in RepositoryDetails.spec.ts
match the backend responses; ensure the route patterns and handler payloads
correspond to the endpoints used by the RepositoryDetails page and that mocks
are registered before page.goto().
In `@frontend/__tests__/e2e/pages/UserDetails.spec.ts`:
- Line 5: The test navigates to a real user via await
page.goto('members/arkid15r', { timeout: 120000 }) but the assertions in
UserDetails.spec.ts expect the mock user data (e.g., "Test User", "Test
Location", "testuser@example.com", "Test Company", "Test Issue"), so either
replace the real navigation with a mocked backend response for the member
GraphQL query or seed the backend with a test user matching those values;
specifically, intercept the GraphQL/member fetch used by the page (or add a
Playwright route handler) to return the mock user object used in the assertions,
or change the page.goto target and assertions to match an actual user in the
backend so the test and data align.
In `@frontend/playwright.config.ts`:
- Line 28: The config currently sets baseURL to process.env.FRONTEND_URL which
may be undefined; update the playwright config to either (a) provide a safe
fallback string when process.env.FRONTEND_URL is missing (e.g., default to
'http://localhost:3000') or (b) validate and throw a clear error early if
FRONTEND_URL is required; locate the baseURL assignment in the Playwright config
(baseURL) and implement the chosen check/assignment so tests never receive an
undefined baseURL.
In `@frontend/src/app/page.tsx`:
- Around line 275-277: The h3 uses conflicting Tailwind utilities (truncate,
text-wrap, md:text-nowrap) which prevents the intended responsive behavior;
update the h3 className to remove 'text-wrap' and the global 'truncate', and
apply truncation only at medium+ breakpoints (e.g., replace the class list with
something like "mb-2 text-lg font-semibold md:truncate") so small screens wrap
normally while medium+ screens truncate; adjust the TruncatedText usage if it
also applies truncation to avoid double-truncation.
🧹 Nitpick comments (10)
frontend/__tests__/e2e/pages/Committees.spec.ts (1)
26-35: Consider the approach for edge-case testing.The "No committees found" test uses route interception to simulate an empty response. When migrating to real backend data, you have two options:
Keep this specific test with mocking — It's acceptable to mock edge cases that are difficult to reproduce with real data (empty states, error conditions).
Use database seeding — If the test infrastructure supports it, seed the database with a known state for this test.
If option 1 is chosen, add a comment explaining why this test retains mocking while others use real data.
frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts (1)
7-7: Consider removing redundant navigation timeout.The Playwright config already sets a global
timeout: 120_000. Specifying the same timeout on individualpage.goto()calls is redundant and adds maintenance overhead if the global value changes.If you intend navigation to have a different timeout than other operations, this is fine. Otherwise, consider relying on the global config.
docker/frontend/Dockerfile.e2e.test (1)
1-1: Pin@playwright/testversion to match base image.The base image uses Playwright
v1.58.0, but line 16 installs@playwright/testwithout a version constraint. This could cause version mismatches between the browser binaries (from the base image) and the test runner, potentially leading to compatibility issues.Proposed fix
-RUN pnpm add `@playwright/test` millify +RUN pnpm add `@playwright/test`@1.58.0 millifyAlso applies to: 16-16
frontend/__tests__/e2e/pages/ProjectsHealthDashboardOverview.spec.ts (1)
7-16: Dashboard tests retain mocking while other tests migrated to real data.This test, like
ProjectHealthDashboardMetricsDetails.spec.ts, still usesmockDashboardCookiesto mock authentication and GraphQL responses. This appears intentional since these tests requireisOwaspStaffauthentication that may not be available in the e2e environment.If this is the intended approach (mock auth-protected pages, use real data for public pages), consider adding a comment to clarify why dashboard tests are excluded from the migration.
docker/frontend/Dockerfile.e2e (1)
21-23: Consider pinning npm and pnpm versions for reproducible builds.Using
npm@latestand unpinned pnpm could lead to inconsistent builds across different build times.💡 Suggested version pinning
RUN --mount=type=cache,target=${NPM_CACHE} \ - npm install --ignore-scripts -g npm@latest --cache ${NPM_CACHE} && \ - npm install --ignore-scripts -g pnpm --cache ${NPM_CACHE} + npm install --ignore-scripts -g npm@10 --cache ${NPM_CACHE} && \ + npm install --ignore-scripts -g pnpm@9 --cache ${NPM_CACHE}frontend/__tests__/e2e/pages/CalendarButton.spec.ts (1)
16-26: Test relies on hardcoded event name; consider making it more resilient.The test expects "Event 1" to exist in the database. While the e2e test environment does load predictable data via
backend/data/nest.dump(configured indocker-compose/e2e/compose.yaml), the test remains brittle—it will fail if the dump is updated or the test runs against a different database state. Consider selecting any available calendar button instead of hardcoding the event name, or document the data dependency explicitly.frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
3-12: Consider usingtest.describe.serialand closing the context.Since tests share a single page instance and depend on navigation state, they should run serially to avoid race conditions. Also, the browser context created in
beforeAllshould be closed inafterAll.Proposed fix
-test.describe('Chapter Details Page', () => { +test.describe.serial('Chapter Details Page', () => { let page: Page + let context: Awaited<ReturnType<typeof browser.newContext>> test.beforeAll(async ({ browser }) => { - const context = await browser.newContext() + context = await browser.newContext() page = await context.newPage() await page.goto('/chapters/rosario', { timeout: 25000 }) }) test.afterAll(async () => { await page.close() + await context.close() })frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
3-12: Close the browser context in afterAll.The browser context created in
beforeAllshould be closed inafterAllto properly release resources.Proposed fix
test.describe.serial('Project Details Page', () => { let page: Page + let context: Awaited<ReturnType<typeof browser.newContext>> test.beforeAll(async ({ browser }) => { - const context = await browser.newContext() + context = await browser.newContext() page = await context.newPage() await page.goto('/projects/nest', { timeout: 25000 }) }) test.afterAll(async () => { await page.close() + await context.close() }).github/workflows/run-ci-cd.yaml (1)
394-397: Redundant port mapping with host networking.When using
--network host, the container shares the host's network stack directly, so-p 3000:3000is ignored. This is harmless but redundant and may cause confusion.🔧 Suggested cleanup
docker run -d --name e2e-nest-frontend \ --network host \ --env-file frontend/.env.e2e.example \ - -p 3000:3000 \ owasp/nest:frontend-e2e-latest sh -c 'pnpm run build && pnpm run start'Note: The same redundancy exists at lines 351-357 for the backend container.
frontend/__tests__/e2e/pages/About.spec.ts (1)
54-61: Brittle assertion: Long inline text is fragile.Asserting against a 400+ character description makes this test fragile - any minor text edit in the database will break it. Consider asserting on a shorter, stable identifier or just verifying the element exists with partial text.
♻️ Suggested approach using partial text
await expect( - page.getByText( - 'This milestone focuses on standardizing how OWASP Board activities are recorded, structured, and published, and on making this information available in structured formats designed for verification, analysis, and automation, as well as programmatically accessible formats via APIs and schemas. The objective is to create a consistent, auditable, and extensible representation of board actions -- including motions, votes, discussions, and decisions so they can be reliably accessed by tools, dashboards, and community members. This milestone reinforces OWASP's commitment to openness, integrity, and community trust by making board-related information more transparent, auditable, and reliable.' - ) + page.getByText('This milestone focuses on standardizing how OWASP Board activities', { exact: false }) ).toBeVisible()
ahmedxgouda
force-pushed
the
e2e/migrate-e2e-tests
branch
from
1bdaeaa to
1d8d5ae
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3591 +/- ##
=======================================
Coverage 96.34% 96.34%
=======================================
Files 512 512
Lines 15826 15826
Branches 2132 2174 +42
=======================================
Hits 15248 15248
Misses 431 431
Partials 147 147
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/__tests__/e2e/pages/About.spec.ts (1)
93-107: Test also depends on/aboutpage state.Similar to the key features test, this will fail if the page hasn't been restored to
/about. Once the navigation issue at lines 79-86 is fixed, this test should work correctly.frontend/__tests__/e2e/components/Footer.spec.ts (1)
25-34: File-scopedtest.usewill affect Desktop tests.The
test.usecall at lines 26-29 is placed at file scope (outside anydescribeblock), which means the iPhone 13 device settings will be applied to all tests in this file, including the Desktop tests above. This contradicts the intent of having separate Desktop and Mobile test suites.Additionally, the Mobile
beforeEach(line 33) is missing the{ timeout: 120000 }option that was added to the Desktop tests for consistency.Proposed fix: Move test.use inside the mobile describe block and add timeout
// Mobile tests (iPhone 13) -test.use({ - ...devices['iPhone 13'], - isMobile: true, -}) - test.describe('Footer - Mobile (iPhone 13)', () => { + test.use({ + ...devices['iPhone 13'], + isMobile: true, + }) + test.beforeEach(async ({ page }) => { - await page.goto('/') + await page.goto('/', { timeout: 120000 }) })frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (1)
12-28: URL path doesn't match mock data project name.The test navigates to
/projects/dashboard/metrics/nest(line 14) but the mock data hasprojectName: 'Test'(frommockProjectsDashboardMetricsDetailsData). While this works becausemockDashboardCookiesintercepts all GraphQL requests regardless of the URL, it creates confusion for maintainers.Consider either:
- Using a consistent URL path like
/projects/dashboard/metrics/testto match the mock data, or- Updating the mock data's
projectNameto'nest'(or similar) to match the URLAdditionally, like
ProjectsHealthDashboardOverview.spec.ts, this file still uses mock data rather than real backend data. Please verify if dashboard tests are intentionally excluded from the migration scope.frontend/__tests__/e2e/components/Header.spec.ts (1)
63-72: File-scopedtest.usewill affect Desktop tests.Same issue as in
Footer.spec.ts: thetest.usecall at lines 64-67 is placed at file scope, which will apply iPhone 13 device settings to all tests in this file, including the Desktop tests defined earlier.Additionally, the Mobile
beforeEach(line 71) is missing the{ timeout: 120000 }option for consistency with Desktop tests.Proposed fix: Move test.use inside the mobile describe block and add timeout
// Mobile tests (iPhone 13) -test.use({ - ...devices['iPhone 13'], - isMobile: true, -}) - test.describe('Header - Mobile (iPhone 13)', () => { + test.use({ + ...devices['iPhone 13'], + isMobile: true, + }) + test.beforeEach(async ({ page }) => { - await page.goto('/') + await page.goto('/', { timeout: 120000 }) })frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (2)
29-42: Map popup still expects mock chapter name.With live data,
'OWASP Test Chapter'is unlikely to exist. Use Rosario’s name or derive expected labels from backend data.🐛 Minimal update (if Rosario is the seeded chapter)
- const popupButton = page.getByRole('button', { name: 'OWASP Test Chapter' }) + const popupButton = page.getByRole('button', { name: /OWASP Rosario/i })
45-55: Top contributors still hard-coded to mock names.Live data will vary, so these fixed names/avatars will likely fail. Pull expected contributors from backend seed/API, or assert against non-empty contributor entries with stable selectors.
🤖 Fix all issues with AI agents
In `@docker/frontend/Dockerfile.e2e`:
- Around line 1-35: The Dockerfile uses "FROM node:24-alpine AS builder" but
never performs a multi-stage build, which is misleading; either remove the stage
name by changing the base line to "FROM node:24-alpine" or convert the file into
a true multi-stage build: keep the existing stage as a builder and add a final
stage (e.g., "FROM node:24-alpine AS runner") that copies the built output from
the builder stage (using COPY --from=builder ...) and sets the runtime
USER/EXPOSE and a sensible CMD/ENTRYPOINT (or clearly document that
docker-compose supplies the run command); update references to APK/NPM/PNPM
caches accordingly if moving commands between stages.
In `@frontend/__tests__/e2e/pages/CalendarButton.spec.ts`:
- Around line 7-16: The test creates a manual context that bypasses Playwright
project `use` options (so page.goto('/') and acceptDownloads are missing) and it
doesn't close the context; update test.beforeAll to create the context with the
same options used by the project (at minimum include acceptDownloads: true and
the configured baseURL e.g. baseURL: process.env.FRONTEND_URL) when calling
browser.newContext(), ensure you assign that context to the same `context`
variable used to create `page` (used by test.beforeAll/page.goto) and in
test.afterAll call both page.close() and context.close() so downloads captured
by download.path() are available and resources are cleaned up.
In `@frontend/__tests__/e2e/pages/ChapterDetails.spec.ts`:
- Around line 57-63: The "toggle top contributors" test assumes extra
contributors exist and fails when they don't; update the test named "toggle top
contributors" to first determine whether extra contributors are present (either
by querying the contributor list DOM via a locator/count or by reading the
backend/fixture contributor count) and branch accordingly: if the contributor
count is above the threshold, perform the current show-more/show-less clicks and
visibility asserts using the existing page.getByRole('button', { name: ... })
calls; otherwise assert that the "Show more" button is not present (or skip the
toggle assertions). Use the test's existing identifiers (test name "toggle top
contributors" and the page.getByRole selectors) to find where to add the
conditional guard.
- Around line 3-12: The test suite "Chapter Details Page" is currently running
in parallel and shares the same page instance causing flakiness; fix it by
configuring the suite to run serially: inside the test.describe block for
"Chapter Details Page" (the block that defines page, test.beforeAll,
test.afterAll and calls page.goto('/chapters/rosario')), add
test.describe.configure({ mode: 'serial' }) so tests execute sequentially and UI
state mutations (e.g., the tests that unlock the map or toggle "Show more"/"Show
less") cannot race.
- Around line 5-11: The test creates a manual context with browser.newContext()
which doesn't inherit the project-level baseURL, causing relative navigation to
fail, and it only closes the page (leaving the context open); fix by creating
the context with an explicit baseURL (e.g., pass the configured FRONTEND_URL or
the test config base URL into browser.newContext({ baseURL: ... })) inside
test.beforeAll and store the returned context in a variable (e.g., context) used
to create page = await context.newPage(), and in test.afterAll call await
context.close() instead of await page.close(); apply the same change to the
other failing specs (ProjectDetails.spec.ts, About.spec.ts,
CalendarButton.spec.ts).
In `@frontend/__tests__/e2e/pages/ProjectDetails.spec.ts`:
- Around line 3-12: The test creates a browser context in test.beforeAll but
never closes it, leaking resources; update the test to store the created context
in an outer-scope variable (e.g., add let context: BrowserContext alongside let
page: Page), assign it inside test.beforeAll where context = await
browser.newContext(), and then call await context.close() in test.afterAll
before/after awaiting page.close() so both the Page (page) and the
BrowserContext (context) are properly closed; modify references to use the
stored context when creating the page (page = await context.newPage()).
- Line 33: Rename the test title string in the ProjectDetails.spec.ts test case
from "should have project statics block" to "should have project statistics
block"; locate the test(...) invocation in ProjectDetails.spec.ts that contains
that exact title and update the string literal (and any other places in the same
file or test suite that reference the old phrase) so the name reads "statistics"
instead of "statics".
- Around line 88-101: The 'should have project repositories' test navigates away
by clicking the repository link (page.getByText('www-project-nest') click and
expect(page).toHaveURL(...)) which breaks the next test due to shared page
state; fix by restoring the Project Details page at the end of that test — after
asserting the URL for the repository (the click/assertion around
'www-project-nest' and
expect(page).toHaveURL('organizations/OWASP/repositories/www-project-nest')),
add a navigation back to '/projects/nest' (e.g., page.goto or clicking a
breadcrumb) so the following 'should display health metrics section' test runs
on the original page.
🧹 Nitpick comments (6)
frontend/__tests__/e2e/pages/Login.spec.ts (1)
5-5: Consider reducing the navigation timeout.A 2-minute (120000ms) timeout for page navigation is excessive and could mask performance regressions. Even with real backend startup overhead, 60 seconds should be sufficient. Long timeouts also slow down CI feedback when tests genuinely fail.
Suggested timeout reduction
- await page.goto('/auth/login', { timeout: 120000 }) + await page.goto('/auth/login', { timeout: 60000 })If the 2-minute timeout is intentional due to known infrastructure constraints (e.g., cold container startup), consider documenting this with a comment explaining why.
frontend/__tests__/e2e/pages/About.spec.ts (2)
54-61: Fragile assertion on full milestone description.Asserting on the entire multi-paragraph description is brittle - any minor text edit will break the test. Consider asserting on the milestone title only, or use a partial text match:
- await expect( - page.getByText( - 'This milestone focuses on standardizing how OWASP Board activities are recorded, structured, and published, and on making this information available in structured formats designed for verification, analysis, and automation, as well as programmatically accessible formats via APIs and schemas. The objective is to create a consistent, auditable, and extensible representation of board actions -- including motions, votes, discussions, and decisions so they can be reliably accessed by tools, dashboards, and community members. This milestone reinforces OWASP's commitment to openness, integrity, and community trust by making board-related information more transparent, auditable, and reliable.' - ) - ).toBeVisible() + await expect( + page.getByText('This milestone focuses on standardizing how OWASP Board activities') + ).toBeVisible()
29-31: Test only verifies heading, not actual leader data.The test name says "displays leaders data when data is loaded" but only checks that the heading exists. Consider adding assertions for actual leader content (e.g., leader names or count).
frontend/__tests__/e2e/components/Header.spec.ts (1)
47-49: Redundant navigation in test.The
beforeEachhook already navigates to'/'before each test. This additionalpage.goto('/')call is redundant and adds unnecessary delay.Proposed fix
test('all dropdown triggers should use pointer cursor', async ({ page }) => { - await page.goto('/') - const dropdownButtons = page.locator('#navbar-sticky button')docker/frontend/Dockerfile.e2e (1)
14-22: Potential permission issues with global npm/pnpm installation.The global npm and pnpm installations (lines 20-22) run as root, but the container later switches to
USER node(line 34). The pnpm store at/pnpm/storeis also populated while running as root duringpnpm install(line 26).This could cause permission issues if the node user needs to modify the pnpm store at runtime. Consider either:
- Running the installations as the node user, or
- Ensuring proper ownership is set on
/pnpmbefore switching usersProposed fix
RUN --mount=type=cache,id=pnpm,target=/pnpm/store \ - pnpm install --frozen-lockfile --ignore-scripts + pnpm install --frozen-lockfile --ignore-scripts && \ + chown -R node:node /app COPY --chmod=444 .pnpmrc next.config.ts postcss.config.js tailwind.config.mjs tsconfig.json ./frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
14-20: Avoid asserting the entire live summary string.This long exact match is brittle when the backend content changes. Prefer a stable snippet or regex.
♻️ Example stabilization
- await expect( - page.getByText( - 'The OWASP Rosario chapter is located in Argentina, South America. It is a part of the OWASP community, which focuses on improving software security. The chapter encourages local members to participate in various projects, events, and discussions related to application security. Although the exact age and size of the chapter are not specified, it is part of a larger organization that welcomes everyone interested in learning about software security. Members can also become involved by attending meetups, contributing to projects, and networking with others. The chapter promotes diversity and aims to create a supportive environment for all participants. Overall, it provides a platform for individuals to enhance their knowledge and reputation in the field of application security.' - ) - ).toBeVisible() + await expect( + page.getByText(/OWASP Rosario chapter is located in Argentina/i) + ).toBeVisible()
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docker-compose/e2e/compose.yaml`:
- Around line 92-119: The frontend service healthcheck uses a non-routable
destination (http://0.0.0.0:3000/) in the healthcheck test which can fail;
update the frontend healthcheck (service name "frontend", healthcheck block,
test command) to target localhost or 127.0.0.1 (e.g., http://localhost:3000/ or
http://127.0.0.1:3000/) so the wget --spider request reliably reaches the
running server.
In `@frontend/.env.e2e.example`:
- Around line 1-13: Update the e2e example environment to match other test
targets by changing NEXT_PUBLIC_ENVIRONMENT from "production" to "local" and
setting NEXT_SERVER_DISABLE_SSR from "true" to "false" in
frontend/.env.e2e.example; locate and edit the NEXT_PUBLIC_ENVIRONMENT and
NEXT_SERVER_DISABLE_SSR entries so e2e runs with local environment and SSR
enabled to avoid masking SSR regressions.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
69-75: Fixture destructuring shadows the sharedpage– test will fail.Destructuring
{ page }from the test callback creates a fresh fixture page that hasn't navigated to the repository URL. This shadows the sharedpagevariable from line 5, causing the test to operate on a blank page where "Show more" won't exist.Remove the destructuring to use the shared page:
🐛 Proposed fix
- test('toggle top contributors', async ({ page }) => { + test('toggle top contributors', async () => {frontend/__tests__/e2e/components/Header.spec.ts (1)
101-120: Menu toggle state causes test failure in serial execution.In serial mode with a shared page, the menu opened in
should show button when menu clicked(line 103) remains open. Whenshould show navigation when menu clicked(line 109) clicks the menu button again, it will close the menu instead of opening it, causing assertions at lines 111-119 to fail.🐛 Suggested fix: remove redundant menu click or reset state
Option 1: Remove the redundant click since menu is already open:
test('should show navigation when menu clicked', async () => { - const menuButton = page.getByRole('button', { name: /menu/i }) - await menuButton.click() - await expect(page.getByRole('banner').getByRole('link', { name: 'Chapters' })).toBeVisible()Option 2: Close the menu at the end of the first test to reset state:
test('should show button when menu clicked', async () => { const menuButton = page.getByRole('button', { name: /menu/i }) await menuButton.click() await expect(page.getByRole('link', { name: 'Star On Github' })).toBeVisible() await expect(page.getByRole('link', { name: 'Sponsor Us' })).toBeVisible() + await menuButton.click() // Close menu to reset state })
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/components/Header.spec.ts`:
- Around line 82-86: The mobile test's setup in test.beforeAll uses
page.goto('/') without a timeout, causing flakiness; update the test.beforeAll
block (the one that calls context = await browser.newContext(), page = await
context.newPage(), await page.goto('/')) to pass the same 25_000ms timeout used
by the desktop beforeAll (e.g. await page.goto('/', { timeout: 25000 })) so the
navigation has the consistent 25-second timeout.
In `@frontend/__tests__/e2e/pages/CommitteeDetails.spec.ts`:
- Around line 28-37: CSpell is flagging the name "Tarandach" used in the
leadersText regex (/Leaders:.*Izar Tarandach, Allison Shubert, Maria Mora/i);
fix by either adding "Tarandach" to the project cspell dictionary (e.g., add
"Tarandach" to the "words" array in cspell.json) or add a local inline ignore
above the test (e.g., a comment like // cspell:ignore Tarandach) so the
CommitteeDetails.spec.ts leadersText check no longer triggers the spellcheck
failure.
In `@frontend/__tests__/e2e/pages/Home.spec.ts`:
- Around line 68-75: The spell-check failure is caused by the domain "glueup"
used in the test 'should be able to join OWASP' (see the joinLink assertion and
the href regex); fix by adding "glueup" to the project's CSpell dictionary
(e.g., add the word to the "words" array in cspell.json) or, if you prefer a
file-scoped change, add an inline CSpell ignore comment at the top of
frontend/__tests__/e2e/pages/Home.spec.ts that exempts "glueup" from checking so
the expectation using joinLink and toHaveAttribute('/owasp.glueup.com/') no
longer fails CI.
In `@frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts`:
- Around line 7-15: The manual context created in test.beforeAll ignores
project-level "use" options (like baseURL), so replace the direct
browser.newContext() call with one that merges the project's use options into
the new context; retrieve the project's use settings via test.info().project.use
and call browser.newContext(projectUseOptions) (update the test.beforeAll that
creates context and page, and keep test.afterAll closing context) so
page.goto('/organizations/OWASP') uses the configured baseURL in CI.
🧹 Nitpick comments (3)
frontend/__tests__/e2e/components/Header.spec.ts (1)
73-76: Move device configuration inside the mobile describe block.The file-scope
test.use()at lines 73-76 applies to all tests in the file. While the desktop block has its owntest.use()override, placing mobile device settings at file scope is error-prone and could cause unexpected behavior if the scoping rules change or new tests are added.♻️ Suggested refactor
-// Mobile tests (iPhone 13) -test.use({ - ...devices['iPhone 13'], - isMobile: true, -}) - test.describe.serial('Header - Mobile (iPhone 13)', () => { let page: Page let context: BrowserContext + test.use({ + ...devices['iPhone 13'], + isMobile: true, + }) + test.beforeAll(async ({ browser }) => {frontend/__tests__/e2e/components/Footer.spec.ts (2)
36-39: File-scopedtest.use()affects all tests, not just mobile.The
test.use()call at lines 36-39 is placed at file scope between the twodescribeblocks, which means it applies to the entire file, including the Desktop tests above. While the Desktop block has its owntest.use()inside, this ordering and scoping is confusing and may lead to unexpected behavior.Move the mobile device configuration inside the mobile
describeblock:Suggested fix
}) // Mobile tests (iPhone 13) -test.use({ - ...devices['iPhone 13'], - isMobile: true, -}) - test.describe.serial('Footer - Mobile (iPhone 13)', () => { + test.use({ + ...devices['iPhone 13'], + isMobile: true, + }) + let page: Page let context: BrowserContext
45-49: Consider adding a timeout for consistency.The Desktop tests use
{ timeout: 25000 }forpage.goto(), but the Mobile tests don't specify a timeout. For consistency and to avoid potential flakiness in CI, consider adding the same timeout.Suggested fix
test.beforeAll(async ({ browser }) => { context = await browser.newContext() page = await context.newPage() - await page.goto('/') + await page.goto('/', { timeout: 25000 }) })
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/Home.spec.ts`:
- Around line 35-37: After calling page.goBack() (seen after
firstChapter.click()), add an explicit await to wait for navigation completion
(e.g., await page.waitForURL(...) or await page.waitForLoadState('load')) to
ensure the page is fully loaded before the next test step; apply the same change
to the projects test that also calls page.goBack() so both places use an
explicit wait after goBack().
🧹 Nitpick comments (2)
.github/workflows/run-ci-cd.yaml (2)
348-354: Redundant port mapping with host network mode.When using
--network host, the-p 3000:3000flag is ignored because the container shares the host's network namespace directly. The frontend will still be accessible on port 3000, but the port mapping is unnecessary.♻️ Suggested fix
- name: Run frontend in the background run: | docker run -d --name e2e-nest-frontend \ --network host \ --env-file frontend/.env.e2e.example \ - -p 3000:3000 \ owasp/nest:frontend-e2e-latest
366-373: Missing cache configuration for e2e test image build.Unlike other test image builds in this workflow (e.g., unit test image at lines 229-231, frontend e2e image at lines 337-339), this build step lacks
cache-fromconfiguration. This will cause full rebuilds on every CI run, potentially adding unnecessary build time.♻️ Suggested fix – add cache configuration
- name: Build e2e testing image uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 with: + cache-from: | + type=gha + type=registry,ref=owasp/nest:test-frontend-e2e-cache context: frontend file: docker/frontend/Dockerfile.e2e.test load: true platforms: linux/amd64 tags: owasp/nest:test-frontend-e2e-latest
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
frontend/__tests__/e2e/components/Header.spec.ts (2)
73-76:⚠️ Potential issue | 🟡 MinorMove
test.use()inside the mobile describe block for consistency.The desktop tests have
test.use()scoped inside theirtest.describe.serialblock (lines 8-11), but the mobiletest.use()is placed outside. This inconsistency could lead to unexpected behavior if test order changes or additional tests are added. Move it inside the mobile describe block to match the desktop pattern.Suggested fix
-// Mobile tests (iPhone 13) -test.use({ - ...devices['iPhone 13'], - isMobile: true, -}) - test.describe.serial('Header - Mobile (iPhone 13)', () => { let page: Page let context: BrowserContext + test.use({ + ...devices['iPhone 13'], + isMobile: true, + }) + test.beforeAll(async ({ browser }) => {
101-120:⚠️ Potential issue | 🟠 MajorPotential issue: Menu toggle state between serial tests.
Both tests click the menu button on the shared page. Since tests run serially with persistent state:
- Test at lines 101-106 clicks menu → opens it
- Test at lines 107-120 clicks menu → may close it (toggle behavior)
This could cause the second test to fail because the navigation links won't be visible after the menu closes.
Consider either:
- Resetting menu state between tests
- Combining these into one test
- Checking menu state before clicking
Option 1: Check and ensure menu is closed before clicking
test('should show navigation when menu clicked', async () => { const menuButton = page.getByRole('button', { name: /menu/i }) + // Ensure menu is closed before clicking (reset state from previous test) + const chaptersLink = page.getByRole('banner').getByRole('link', { name: 'Chapters' }) + if (await chaptersLink.isVisible()) { + await menuButton.click() // Close menu first + } await menuButton.click()Option 2: Navigate fresh at start of test
test('should show navigation when menu clicked', async () => { + await page.goto('/', { timeout: 25000 }) const menuButton = page.getByRole('button', { name: /menu/i }) await menuButton.click()frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts (1)
72-77:⚠️ Potential issue | 🟡 MinorDate assertion may be flaky due to timezone-dependent formatting.
toLocaleStringwithout atimeZoneoption uses the system's local timezone. The mock data has'2023-01-01T00:00:00Z'(midnight UTC), which could format as "Dec 31, 2022" on machines west of UTC versus "Jan 1, 2023" on UTC. This can cause flaky tests on CI runners in different timezones.🛡️ Proposed fix: Specify UTC timezone
new Date(firstMetric.createdAt).toLocaleString('en-US', { month: 'short', day: 'numeric', year: 'numeric', + timeZone: 'UTC', })frontend/__tests__/e2e/pages/Projects.spec.ts (1)
29-33:⚠️ Potential issue | 🔴 CriticalCritical: Test uses fixture
pageinstead of sharedpage, bypassing route mock.This test (and others at lines 35, 46, 53, 59) destructures
{ page }from the test fixture, which shadows the outerpagevariable created inbeforeAll. The route mock set up on the sharedpagewon't apply to this fixture page, causing the test to hit real endpoints or fail.Remove the
{ page }parameter to use the shared page with the mock route.🐛 Proposed fix
- test('renders project data correctly', async ({ page }) => { + test('renders project data correctly', async () => { await expect(page.getByRole('link', { name: 'Project 1' })).toBeVisible() await expect(page.getByText('This is a summary of Project 1')).toBeVisible() await expect(page.getByRole('button', { name: 'View Details' })).toBeVisible() }) - test('displays "No Projects found" when there are no projects', async ({ page }) => { + test('displays "No Projects found" when there are no projects', async () => { await page.route('**/idx/', async (route) => { ... - test('handles page change correctly', async ({ page }) => { + test('handles page change correctly', async () => { ... - test('opens window on View Details button click', async ({ page }) => { + test('opens window on View Details button click', async () => { ... - test('breadcrumb renders correct segments on /projects', async ({ page }) => { + test('breadcrumb renders correct segments on /projects', async () => {frontend/__tests__/e2e/pages/Users.spec.ts (1)
34-43:⚠️ Potential issue | 🔴 CriticalTest mutates shared page state without restoration.
This test navigates to
/memberswith an empty mock response. Since this is a serial suite with shared page state, subsequent tests (page change, button click, counts) will operate on this empty-results page rather than the original mocked data, causing them to fail.Either:
- Restore the original route mock and navigate back after this test, or
- Move this test to the end of the suite.
🐛 Proposed fix: restore state after test
test('displays "No user found" when there are no users', async () => { await page.route('**/idx/', async (route) => { await route.fulfill({ status: 200, body: JSON.stringify({ hits: [], nbPages: 0 }), }) }) await page.goto('/members') await expect(page.getByText('No Users Found')).toBeVisible() + // Restore original mock for subsequent tests + await page.route('**/idx/', async (route) => { + await route.fulfill({ + status: 200, + body: JSON.stringify({ + hits: mockUserData.users, + nbPages: 2, + }), + }) + }) + await page.goto('/members') })frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
71-77:⚠️ Potential issue | 🔴 CriticalTest receives page fixture instead of using shared page.
This test signature
async ({ page })destructures a fresh page from the Playwright fixture, shadowing the module-scopepagevariable. This breaks the serial test pattern—the fixture page won't be on the repository details page, causing assertions to fail.Remove the fixture parameter to use the shared page.
🐛 Proposed fix
- test('toggle top contributors', async ({ page }) => { + test('toggle top contributors', async () => { await expect(page.getByRole('button', { name: 'Show more' })).toBeVisible() await page.getByRole('button', { name: 'Show more' }).click() await expect(page.getByRole('button', { name: 'Show less' })).toBeVisible() await page.getByRole('button', { name: 'Show less' }).click() await expect(page.getByRole('button', { name: 'Show more' })).toBeVisible() })frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
35-51:⚠️ Potential issue | 🟠 MajorUnroute the empty-data handler after the "No Issues found" test to prevent state leakage.
With a shared
Pageand sequential tests, callingpage.route('**/idx/', ...)again in this test overrides the default mock frombeforeAll. Since the second handler is never unrouted, subsequent tests that depend on the paginated mock data (pagination test, dialog test) will receive empty data instead, causing test order dependencies and failures.🧹 Proposed fix
test('displays "No Issues found" when there are no issues', async () => { + const emptyIssuesRoute = async (route) => { + await route.fulfill({ + status: 200, + body: JSON.stringify({ hits: [], totalPages: 0 }), + }) + } - await page.route('**/idx/', async (route) => { - await route.fulfill({ - status: 200, - body: JSON.stringify({ hits: [], totalPages: 0 }), - }) - }) + await page.route('**/idx/', emptyIssuesRoute) await page.goto('/contribute') await expect(page.getByText('No issues found')).toBeVisible() + await page.unroute('**/idx/', emptyIssuesRoute) })
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/Projects.spec.ts`:
- Line 5: Tests in Projects.spec use test.describe but must use
test.describe.serial to guarantee ordered execution when sharing page/context;
update the top-level test.describe('Projects Page', ...) to
test.describe.serial('Projects Page', ...) so the suite runs serially, and
ensure any imports still reference the same test object (e.g., the Playwright
"test" used across other converted files) to maintain consistency with other
specs that rely on shared page/context state.
In `@frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts`:
- Around line 7-15: Playwright route handlers added via page.route() are
cumulative so successive tests using mockDashboardCookies(page, ...) register
competing handlers and the first registered wins; fix by clearing existing
routes before adding new ones—either call page.unroute(...) or recreate the
context/page in beforeEach, or implement and use a new helper
setupMocks(mockHealthMetricsData, isOwaspStaff) that first removes existing
routes from the shared page (or reinitializes the page) and then registers the
route handlers (replace calls to mockDashboardCookies with setupMocks in each
test); reference page.route, page.unroute, mockDashboardCookies, and setupMocks
when making the change.
In `@frontend/__tests__/e2e/pages/Users.spec.ts`:
- Around line 65-67: The breadcrumb test in test('breadcrumb renders correct
segments on /members') can flake if prior tests left the browser at
/members/user_1; before calling expectBreadCrumbsToBeVisible(page, ['Home',
'Members']) ensure the page is explicitly directed to the members index (e.g.,
navigate via page.goto or a shared navigateToMembers helper) so the test starts
from /members; update the test in Users.spec.ts to perform that navigation using
the existing page fixture before asserting with expectBreadCrumbsToBeVisible.
- Around line 52-57: The test "opens window on button click" navigates to
/members/user_1 and breaks subsequent tests; after the assertion in the test
(the expect(page).toHaveURL('/members/user_1') call) navigate back to the
members listing (e.g., call page.goto('/members') or page.goBack()) and wait for
the members list to be visible (use page.waitForSelector or
page.waitForURL('/members')) so later tests that expect the listing page start
from the correct URL.
- Line 2: Users.spec.ts still imports mockUserData and intercepts the /idx/
network call via page.route(), which contradicts the PR goal to use real
backend/database data; remove the mock import (mockUserData) and any
page.route() interception for the /idx/ endpoint, then update the test
assertions in Users.spec.ts to validate against actual backend responses (or
query the test database fixture) instead of mocked values, or add a short
comment explaining why this test must remain mocked as an explicit exception if
backend data cannot be used.
🧹 Nitpick comments (9)
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (1)
19-19: Consider extracting the hardcoded timeout to a shared constant.The 25000ms timeout is duplicated across
page.gotocalls. Extracting this to a named constant (e.g.,const NAVIGATION_TIMEOUT = 25000) would improve readability and make it easier to adjust consistently across tests.♻️ Suggested refactor
+const NAVIGATION_TIMEOUT = 25000 + test.describe.serial('Project Health Metrics Details Page', () => { let page: Page let context: BrowserContext // ... hooks ... test('renders 404 when user is not OWASP staff', async () => { await mockDashboardCookies(page, mockProjectsDashboardMetricsDetailsData, false) - await page.goto('/projects/dashboard/metrics/test-project', { timeout: 25000 }) + await page.goto('/projects/dashboard/metrics/test-project', { timeout: NAVIGATION_TIMEOUT }) // ... }) test('renders project health metrics details', async () => { await mockDashboardCookies(page, mockProjectsDashboardMetricsDetailsData, true) - await page.goto('/projects/dashboard/metrics/nest', { timeout: 25000 }) + await page.goto('/projects/dashboard/metrics/nest', { timeout: NAVIGATION_TIMEOUT }) // ... }) })Also applies to: 25-25
frontend/__tests__/e2e/components/Header.spec.ts (1)
56-69: Redundant navigation within serial test suite.Since
beforeAllalready navigates to/and tests run serially with a shared page, thispage.goto('/')is redundant. If the intent is to reset page state after the previous test's dropdown interaction, consider documenting it with a comment. Otherwise, remove it for consistency with other tests in this suite.frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts (1)
18-18: Consider extracting the repeated timeout value to a constant.The
25000timeout is used in all threepage.goto()calls. Extracting it to a named constant improves readability and makes future adjustments easier.♻️ Suggested refactor
+const NAVIGATION_TIMEOUT = 25000 + test.describe.serial('Projects Health Dashboard Metrics', () => { // ... - await page.goto('/projects/dashboard/metrics', { timeout: 25000 }) + await page.goto('/projects/dashboard/metrics', { timeout: NAVIGATION_TIMEOUT })frontend/playwright.config.ts (1)
32-32: Consider reducingworkerswhen running serial tests.With
fullyParallel: falseandtest.describe.serialblocks, the high worker count (os.cpus().length) provides limited benefit since tests within a serial block run sequentially. Consider settingworkers: 1or a lower count to reduce resource overhead.♻️ Optional: Reduce workers for serial execution
- workers: os.cpus().length, + workers: 1,frontend/__tests__/e2e/pages/ProjectsHealthDashboardOverview.spec.ts (1)
6-6: Consider adding.serialfor consistency.Other converted test files use
test.describe.serial(...). While these tests work independently, adding.serialwould maintain consistency across the test suite.♻️ Optional: Add serial modifier
-test.describe('Projects Health Dashboard Overview', () => { +test.describe.serial('Projects Health Dashboard Overview', () => {frontend/__tests__/e2e/pages/Committees.spec.ts (1)
41-41: Consider adding timeout for consistency.This
gotocall lacks the{ timeout: 25000 }option used in thebeforeAllnavigation (line 22). For consistency and reliability, consider adding the same timeout.♻️ Suggested fix
- await page.goto('/committees') + await page.goto('/committees', { timeout: 25000 })frontend/__tests__/e2e/pages/UserDetails.spec.ts (1)
31-42: Static analysis flag is a false positive for this use case.The ast-grep ReDoS warning about
new RegExp(\\d.*${stat}`)is not a concern here becausestatvalues are hardcoded trusted strings ('Followers','Following','Repositories'`), not user input. The pattern is safe.However, for cleaner code, you could use a static regex with alternation:
♻️ Optional: Use static regex pattern
test('should have user stats block', async () => { - // Validation of stats grid using regex to handle changing numbers - const stats = ['Followers', 'Following', 'Repositories'] - for (const stat of stats) { - await expect( - page - .locator('div') - .filter({ hasText: new RegExp(`\\d.*${stat}`) }) - .first() - ).toBeVisible() - } + // Validation of stats grid + await expect(page.getByText(/\d+\s*Followers/)).toBeVisible() + await expect(page.getByText(/\d+\s*Following/)).toBeVisible() + await expect(page.getByText(/\d+\s*Repositories/)).toBeVisible() })frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
104-104: URL assertion missing leading slash.The URL assertion uses a relative path without a leading slash. While Playwright's
toHaveURLcan match substrings, this is inconsistent with standard URL conventions. Consider adding the leading slash for clarity.✏️ Proposed fix
- await expect(page).toHaveURL('organizations/OWASP/repositories/www-project-nest') + await expect(page).toHaveURL('/organizations/OWASP/repositories/www-project-nest')frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
8-22: Consider if/idx/mocking aligns with test intent.
page.route('**/idx/')mocks the endpoint with controlled data, but this pattern is consistent across all similar E2E tests (Users, Projects, Organizations, Committees, Chapters). If these tests are meant to verify UI rendering with predictable data, mocking is appropriate. However, if the goal is integration testing with real backend responses, document why the mock is necessary or migrate to backend seeding.Also note: the "No Issues found" test (lines 35-41) adds another route handler to the same shared page; verify this doesn't cause state leakage across subsequent tests.
frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (1)
17-42:⚠️ Potential issue | 🟡 MinorRoute handlers may accumulate between serial tests, causing unexpected behavior.
When using a shared
pageacross serial tests, callingmockDashboardCookies(page, ...)in each test adds newpage.route()handlers. In Playwright, route handlers accumulate—the second test's handlers do not replace the first test's handlers; both remain active. The first matching handler wins, so the second test could still receive responses configured by the first test.To ensure proper isolation between tests within the serial suite, consider one of these approaches:
- Unroute before re-mocking (preferred for shared page):
await page.unroute('**/api/auth/session') await page.unroute('**/graphql') await mockDashboardCookies(page, ...)
- Clear cookies between tests (if cookies carry auth state):
await page.context().clearCookies()
- Navigate to a blank page or reload before setting up new mocks to reset page state.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/CalendarButton.spec.ts`:
- Around line 9-15: The context is created in test.beforeAll without the
Playwright acceptDownloads setting so download.path() can be null; update the
context creation (the context variable instantiated inside test.beforeAll) to
include acceptDownloads: true (or better, read it from
testInfo.project.use.acceptDownloads) in the newContext options so downloads are
captured before calling page.goto and before any download.path() checks.
In `@frontend/__tests__/e2e/pages/Chapters.spec.ts`:
- Around line 35-40: The pagination test ('handles page change correctly')
mutates the shared page state by navigating to ?page=2; after clicking
nextPageButton add a reset step to return the shared page to the initial state
(e.g., navigate or reload to the base chapters URL or to ?page=1 and wait for
stability) so subsequent tests in test.describe.serial() see the expected
initial page; reference the page object and the test name when adding this
cleanup. Ensure the reset runs after the click and URL assertion so it does not
interfere with the test's verification.
In `@frontend/__tests__/e2e/pages/Committees.spec.ts`:
- Around line 50-59: The route mock for Playwright is registered after
navigation, so move the call to page.route(...) to before
page.goto('/committees') in the test titled 'displays "No committees found" when
there are no committees' so the network request to '**/idx/' is intercepted;
ensure the route handler (the async route.fulfill returning { status: 200, body:
JSON.stringify({ hits: [], nbPages: 0 }) }) is set up prior to calling page.goto
and then assert page.getByText('No committees found') as before.
In `@frontend/__tests__/e2e/pages/Contribute.spec.ts`:
- Around line 55-59: The test "closes dialog on close button click" currently
clicks closeButton but doesn't assert the dialog was dismissed; after clicking
the button (closeButton variable from page.getByRole('button', { name:
'close-modal' })), add an assertion that the dialog element is not present or
not visible (e.g., locate the dialog via page.getByRole('dialog') or the
specific dialog locator used in this spec and use expect(...).not.toBeVisible()
or expect(...).toBeHidden()/toHaveCount(0) to ensure the modal was actually
closed). Ensure the assertion waits for the UI change (use Playwright's expect
matcher which auto-waits) so the test fails if the dialog remains open.
In `@frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts`:
- Around line 71-75: The avatar assertion in the "should display top
contributors section" test uses a global selector (img[alt*="avatar"]) so it can
match avatars outside the Top Contributors section; scope the avatar lookup to
the Top Contributors section by locating the section heading via
page.getByRole('heading', { name: 'Top Contributors' }) and then querying for
the avatar inside that section (e.g., use the heading locator's parent/section
and then .locator('img[alt*="avatar"]') or similar) before asserting visibility
on the first avatar.
In `@frontend/__tests__/e2e/pages/Organizations.spec.ts`:
- Around line 44-47: The test 'navigation to organization details works' is
missing awaits and uses a weak synchronous URL check; update the test to await
the click on page.getByRole('button', { name: 'View Profile' }).first().click(),
then use Playwright's matcher await expect(page).toHaveURL(...) (not
expect(page.url())) so the assertion auto-waits; strengthen the assertion to
match the organization detail pattern (e.g., a regex or exact path like
/organizations/:id or contains '/organizations/' plus an id) to ensure
navigation actually reached the detail page.
In `@frontend/__tests__/e2e/pages/ProjectDetails.spec.ts`:
- Around line 66-72: The test "toggle top contributors" is clicking buttons
without awaiting the Promise, causing race conditions; update the two calls to
page.getByRole(...).last().click() and page.getByRole(..., { name: 'Show less'
}).click() to await their results so the click completes before the next
assertion (i.e., add await before both .click() invocations in the toggle top
contributors test).
In `@frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts`:
- Around line 70-75: In the "toggle top contributors" test, the two calls to
page.getByRole('button', { name: 'Show more' }).last().click() and
page.getByRole('button', { name: 'Show less' }).last().click() are not awaited;
make both click() calls awaited so the test waits for the UI action to complete
before running the subsequent assertions (update the clicks in the test named
"toggle top contributors" to use await page.getByRole(...).last().click()).
In `@frontend/__tests__/e2e/pages/Users.spec.ts`:
- Around line 41-46: The test "displays followers and repositories counts
correctly" depends on page 1 state but runs after "handles page change
correctly", so before asserting for 'John Doe' and '1k'/'2k' you must explicitly
navigate or reset to page 1; update the test (test name: displays followers and
repositories counts correctly) to perform an explicit navigation/reset (e.g.,
click the pagination control for page "1" or call page.goto('/users?page=1') or
trigger whatever reset helper exists) and wait for the user list to be visible
(use the existing page.getByRole/getByText selectors) before performing the
expect assertions so it no longer relies on prior test order.
- Around line 34-39: The test "handles page change correctly" mutates shared
page state by navigating to page 2; after the assertion add a deterministic
reset to page 1 so subsequent serial tests see the expected initial state — e.g.
after await expect(page).toHaveURL(/page=2/) add a reset such as awaiting a
click on the page-1 control (await page.getByRole('button', { name: 'Go to page
1' }).click()) or navigate directly (await page.goto('/?page=1') or appropriate
users URL) to return the shared page instance to page 1; keep the change inside
the same test (referencing nextPageButton and page) so ordering need not change.
🧹 Nitpick comments (10)
frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
51-57: Test is coupled to specific contributor data.The test assumes "Tomas Illuminati Balbin" is a top contributor for the Rosario chapter. This is acceptable for e2e tests against a real backend, but be aware that changes to contributor data in the database will cause test failures.
Consider documenting the expected test data state or using a seeded/known test dataset to ensure stability.
frontend/__tests__/e2e/pages/UserDetails.spec.ts (1)
49-51: Consider a more precise selector for the repository link.The current selector
page.locator('div').filter({ hasText: 'Repositories' })may match multiple elements (the heading, stats section, or the actual repos list). If the page structure changes, this could become flaky.Consider scoping more tightly, for example by targeting the section following the "Repositories" heading:
- const firstRepo = page.locator('div').filter({ hasText: 'Repositories' }).locator('a').first() + const reposSection = page.getByRole('heading', { name: 'Repositories' }).locator('..').locator('..') + const firstRepo = reposSection.locator('a').first()Alternatively, if the repositories section has a unique test-id or class, using that would be more robust.
frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
70-71: Consider adding a timeout for consistency with other navigations.Line 22 uses
{ timeout: 25000 }for navigation, but thisgotocall has no timeout. For stability in CI environments, consider using a consistent timeout.♻️ Suggested change
- await page.goto('/contribute') + await page.goto('/contribute', { timeout: 25000 })frontend/__tests__/e2e/pages/Home.spec.ts (1)
28-35: Consider a more robust locator strategy.The current locator
locator('div').filter({ hasText: 'New Chapters' })may match parent containers that also contain the heading text, potentially selecting unintended links. A more precise approach would scope relative to the heading.♻️ Alternative locator approach
- const firstChapter = page - .locator('div') - .filter({ hasText: 'New Chapters' }) - .locator('a') - .first() + const chaptersSection = page.getByRole('heading', { name: 'New Chapters' }).locator('..') + const firstChapter = chaptersSection.locator('a').first()Or if test IDs are available:
const firstChapter = page.locator('[data-testid="new-chapters-section"] a').first()frontend/__tests__/e2e/pages/Committees.spec.ts (1)
41-43: Breadcrumb test depends on prior test's URL state.Since tests are serial, this test runs after
handles page change correctly, which leaves the page at/committees?page=2. If the intent is to test breadcrumbs on the base/committeesroute, consider adding explicit navigation. If testing breadcrumbs on page 2 is intentional, this is fine as-is..github/workflows/run-ci-cd.yaml (3)
348-354: Port mapping is redundant with--network host.When using
--network host, the container shares the host's network namespace directly, so the-p 3000:3000port mapping has no effect. While this doesn't break functionality, it's misleading and should be removed for clarity.♻️ Proposed fix
- name: Run frontend in the background run: | docker run -d --name e2e-nest-frontend \ --network host \ --env-file frontend/.env.e2e.example \ - -p 3000:3000 \ owasp/nest:frontend-e2e-latest
375-382: Consider adding--rmflag to clean up the container.The
docker runcommand doesn't include--rm, so the container will persist after the tests complete. While CI runners are ephemeral, adding--rmis a good practice for cleanup and consistency with other test containers in this workflow.♻️ Proposed fix
- name: Run frontend end-to-end tests run: | - docker run --network host \ + docker run --rm --network host \ -e FRONTEND_URL=http://localhost:3000 \ -e NEXTAUTH_SECRET=secret \ -e NEXTAUTH_URL=http://localhost:3000 \ owasp/nest:test-frontend-e2e-latest npx playwright test
334-346: Missingcache-toconfiguration for frontend e2e image build.Unlike other build steps in this workflow (e.g., frontend unit-testing and a11y-testing images), this build step only has
cache-frombut nocache-to. This means builds won't contribute to the cache, potentially slowing down future CI runs.♻️ Proposed fix
- name: Build frontend image uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 with: cache-from: | type=gha type=registry,ref=owasp/nest:frontend-e2e-cache + cache-to: | + type=gha,compression=zstd context: frontend file: docker/frontend/Dockerfile build-args: | ENV_FILE=.env.e2e.example load: true platforms: linux/amd64 tags: owasp/nest:frontend-e2e-latestfrontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
49-60: Consider making topics/languages assertions less brittle.These are still fixed values; if backend data changes, the test will break. Prefer asserting a non‑empty list or validate against seeded fixtures to keep stability.
frontend/__tests__/e2e/pages/Projects.spec.ts (1)
45-50: Make the URL assertion resilient to full URLs.
toHaveURLtypically matches the full URL; a relative string can be brittle withbaseURL. Consider a glob/regex to avoid false negatives.🔧 Suggested tweak
- await expect(page).toHaveURL('projects/project_1') + await expect(page).toHaveURL('**/projects/project_1')
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 713 🏁 Script executed: Length of output: 860 🏁 Script executed: Length of output: 2 🏁 Script executed: Length of output: 11727 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3201 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3776 🏁 Script executed: Length of output: 48
Root Cause1. Serial Test Execution with Cascading Failures:
2. Worker Count Mismatch:
3. Environment Differences:
Why This Matterstest.describe.serial('My Tests', () => {
test('test 1', async () => { /* times out */ })
test('test 2', async () => { /* SKIPPED */ })
test('test 3', async () => { /* SKIPPED */ })
test('test 4', async () => { /* SKIPPED */ })
})One failure = entire suite skipped. SolutionsImmediate Fix: // Change from:
test.describe.serial('About Page', () => {
// To:
test.describe('About Page', () => {
test.describe.configure({ mode: 'parallel' })Better Approach - Make Tests Truly Independent:
test.describe('About Page', () => {
test.beforeEach(async ({ context, page }) => {
await page.goto('/about', { timeout: 25000 })
})
test('test 1', async ({ page }) => {
// Each test gets fresh page state
})
})
// In playwright.config.ts
export default defineConfig({
fullyParallel: true, // Change from false
// ... rest of config
})
workers: process.env.CI ? 4 : os.cpus().length
timeout: 180_000, // 3 minutes instead of 2The shared |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/CalendarButton.spec.ts`:
- Around line 11-15: The event-name extraction is too aggressive: change how
eventName is derived from buttonText (variables firstEventButton, buttonText,
eventName) so you only remove the "Add" prefix and the "to Calendar" suffix
instead of globally replacing those words; update the logic to detect and strip
a leading "Add" (e.g. anchored or startsWith) and a trailing "to Calendar" (e.g.
anchored or endsWith) and then trim, ensuring internal occurrences of those
words inside the event name remain intact.
In `@frontend/__tests__/e2e/pages/Projects.spec.ts`:
- Around line 43-52: The test 'displays "No Projects found" when there are no
projects' is missing the explicit CI timeout used in your beforeEach; update the
final assertion (the expect on page.getByText('No projects
found').toBeVisible()) to include the same timeout option used in beforeEach
(e.g., .toBeVisible({ timeout: <same_ms_value> })) so navigation/waiting is
consistent and prevents flaky CI failures.
In `@frontend/__tests__/e2e/pages/UserDetails.spec.ts`:
- Around line 9-10: The test currently asserts specific user data via
page.getByRole('heading', { name: 'Arkadii Yakovets', exact: true }) and
page.getByText('@arkid15r') but relies implicitly on the committed dump; either
add a clear comment above those assertions noting that this user must be present
in backend/data/nest.dump (so future changes keep it), or better: add an
explicit e2e fixture to reliably create the test user (e.g., add a
beforeAll/beforeEach that calls a test helper like seedTestUser or executes a
SQL insert/HTTP API to create user "Arkadii Yakovets" / "@arkid15r" before
navigating) and tear it down after, ensuring the assertions reference the
guaranteed fixture rather than an undocumented dump entry.
🧹 Nitpick comments (8)
frontend/__tests__/e2e/pages/Home.spec.ts (2)
17-23: Consider anchoring the locator from the heading for stability.Using
filter({ hasText: 'New Chapters' })on genericdivelements may match multiple nested containers (including parent wrappers), which could return unexpected elements depending on DOM structure.A more robust approach anchors from the known heading:
♻️ Suggested refactor
- const firstChapter = page - .locator('div') - .filter({ hasText: 'New Chapters' }) - .locator('a') - .first() + const firstChapter = page + .getByRole('heading', { name: 'New Chapters' }) + .locator('..') + .locator('a') + .first()Alternatively, if the section has a specific
data-testidor landmark, use that for more explicit targeting.
28-33: Same locator pattern refinement applies here.Consider the same heading-anchored approach suggested for the chapters test above.
frontend/__tests__/e2e/pages/About.spec.ts (1)
47-63: Long text assertion is fragile; missing assertion after "Show more" click.The 500+ character text assertion at lines 51-53 will break on any minor content edit. Consider asserting on a shorter unique identifier (e.g., the milestone title only) or checking for structural elements.
Additionally, clicking "Show more" at line 62 without a subsequent assertion doesn't verify the expanded content.
♻️ Suggested improvement
await expect( page.getByText('OWASP Board Activity Standardization and Data Programmatic Access') ).toBeVisible() - await expect( - page.getByText( - 'This milestone focuses on standardizing how OWASP Board activities are recorded, structured, and published, and on making this information available in structured formats designed for verification, analysis, and automation, as well as programmatically accessible formats via APIs and schemas. The objective is to create a consistent, auditable, and extensible representation of board actions -- including motions, votes, discussions, and decisions so they can be reliably accessed by tools, dashboards, and community members. This milestone reinforces OWASP's commitment to openness, integrity, and community trust by making board-related information more transparent, auditable, and reliable.' - ) - ).toBeVisible() + // Assert milestone description container is visible instead of exact text + await expect(page.locator('[data-testid="roadmap-item"]').first()).toBeVisible() const roadmapSection = page .locator('div') .filter({ has: page.getByRole('heading', { name: 'Roadmap' }) }) .filter({ has: page.getByRole('button', { name: 'Show more' }) }) .last() await roadmapSection.getByRole('button', { name: 'Show more' }).click() + // Assert expanded content is visible + await expect(roadmapSection.getByRole('button', { name: 'Show less' })).toBeVisible() })frontend/__tests__/e2e/pages/Projects.spec.ts (1)
36-41: Misleading variable name.The variable
contributeButtonactually refers to the "View Details" button. Consider renaming for clarity.✏️ Suggested rename
- const contributeButton = page.getByRole('button', { name: 'View Details' }) - await contributeButton.waitFor({ state: 'visible' }) - await contributeButton.click() + const viewDetailsButton = page.getByRole('button', { name: 'View Details' }) + await viewDetailsButton.waitFor({ state: 'visible' }) + await viewDetailsButton.click()frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts (2)
4-5: Avoid hardcoding a shorter navigation timeout.
A fixed 25s overrides Playwright’s configured navigation timeout and is often too low for CI; consider inheriting project defaults instead.🔧 Suggested change
test.beforeEach(async ({ page }) => { - await page.goto('/organizations/OWASP', { timeout: 25000 }) + await page.goto('/organizations/OWASP') })
18-28: Scope stat-value assertions to the stat row.
The globaldiv+ regex can match unrelated numbers elsewhere on the page; anchor the numeric check to the same container as the label to reduce false positives.🔧 Possible scoping approach
- for (const stat of stats) { - await expect(page.getByText(stat).first()).toBeVisible() - // Verify that there is a number associated with the stat (e.g., "15.4K Stars") - await expect( - page - .locator('div') - .filter({ hasText: new RegExp(`\\d.*${stat}`) }) - .first() - ).toBeVisible() - } + for (const stat of stats) { + const statRow = page.getByText(stat, { exact: true }).first().locator('xpath=..') + await expect(statRow).toBeVisible() + // Verify that there is a number associated with the stat (e.g., "15.4K Stars") + await expect(statRow.getByText(/\d/)).toBeVisible() + }frontend/__tests__/e2e/pages/UserDetails.spec.ts (2)
20-29: Stats regex is permissive—scope or anchor to avoid false positives.
Line 26 matches anydivwith a digit + stat word, which could pass even if the stats grid is missing. Consider anchoring the regex or scoping to the stats container.🔧 Example anchor (adjust if markup differs)
- .filter({ hasText: new RegExp(`\\d.*${stat}`) }) + .filter({ hasText: new RegExp(`^\\s*\\d+\\s+${stat}\\b`) })
32-40: Scope repository lookup to the Repositories section.
Line 39 filters alldivs containing “Repositories,” which might match nav or other panels. Scoping to the specific section makes the assertion more reliable.🔧 Example scoping pattern (adjust to actual DOM)
- const firstRepo = page.locator('div').filter({ hasText: 'Repositories' }).locator('a').first() + const reposSection = page.getByRole('heading', { name: 'Repositories' }).locator('..') + const firstRepo = reposSection.getByRole('link').first()
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docker/frontend/Dockerfile.e2e.test`:
- Around line 16-18: The COPY in Dockerfile.e2e.test is referencing a
non-existent path; update the COPY source to the actual lockfile location or
adjust the build context: change the COPY that references
"__tests__/e2e/pnpm-lock.yaml" to "frontend/__tests__/e2e/pnpm-lock.yaml" (or
ensure the build context includes the frontend/ prefix) so the COPY --chmod=444
__tests__/e2e/package.json __tests__/e2e/pnpm-lock.yaml ./ step succeeds before
RUN pnpm install --frozen-lockfile --ignore-scripts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/UserDetails.spec.ts`:
- Around line 39-41: The test's locator is ambiguous and grabs the outer div
(often the stats section) resulting in checking the AnchorTitle link instead of
actual repository items; update the locator to anchor off the "Repositories"
heading and scope into the repositories grid to look for RepositoryItem buttons
(not anchors). For example, replace the current chain starting at
page.locator('div').filter({ hasText: 'Repositories' }) with a heading-based
locator such as page.getByRole('heading', { name: 'Repositories' }) and then
traverse to its section container (e.g.,
.locator('xpath=ancestor::div[`@class`*="mb-8"][1]') or .locator('..') as
appropriate) and assert on a repository item button (e.g.,
.locator('button').first() or a data-test-id for RepositoryItem) so the
assertion verifies actual repository entries rather than the AnchorTitle link.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/run-ci-cd.yaml:
- Around line 324-339: The "Build frontend image" step using
docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 is missing a
cache-to entry so the build cache is never written back; add a cache-to entry
mirroring cache-from (e.g.,
type=registry,ref=owasp/nest:frontend-e2e-cache,mode=max) alongside the existing
cache-from so the Next.js build cache is pushed after the job finishes, keeping
the rest of the step (context: frontend, file: docker/frontend/Dockerfile,
build-args, load, platforms, tags) unchanged.
- Around line 341-347: The env file uses Docker service hostnames that won't
resolve with --network host; edit frontend/.env.e2e.example and replace any
occurrences of http://backend:9000 (or http://backend:9000/...) with
http://localhost:9000 (preserving the path) for the variables
NEXT_SERVER_CSRF_URL and NEXT_SERVER_GRAPHQL_URL so the frontend container can
reach the backend when run with the host network.
In `@docker/frontend/Dockerfile`:
- Around line 40-41: The COPY of ${ENV_FILE} into ./.env in the Dockerfile will
fail the build if the file is missing from the build context; add a clear
comment above the lines "COPY --chmod=444 .pnpmrc ..." and "COPY --chmod=444
${ENV_FILE} ./.env" referencing the ARG ENV_FILE (default .env) to tell
developers that the specified ENV_FILE must exist in the frontend/ build context
(or be provided via --build-arg) before running docker build, or alternatively
implement a fallback (e.g., copy .env.example when ENV_FILE is absent) if you
prefer runtime resilience.
🧹 Nitpick comments (5)
frontend/__tests__/e2e/pages/UserDetails.spec.ts (1)
13-17: Regex checks verify labels only, not that values are populated.The assertions confirm that "Location:" and "Email:" labels appear but don't verify they have non-empty values. If the intent is to confirm the user has these details filled in, consider a regex like
/Location:\s*\S+/iinstead. Minor concern — current approach is fine if you only need structural validation.frontend/__tests__/e2e/pages/RepositoryDetails.spec.ts (1)
25-36: Static analysis ReDoS warning is a false positive here — but the regex could be tightened.The
statvalues come from a hardcoded constant array, so there's no ReDoS risk from user input. However, the pattern\d.*${stat}uses a greedy.*that may inadvertently match unrelated<div>text containing the stat word far from the number. A non-greedy quantifier or a tighter pattern would be more precise.Optional: tighter regex pattern
- const text = String.raw`\d.*${stat}` + const text = String.raw`\d[\d,.kKmM]*\s*${stat}`This limits the match to a number (with possible formatting characters like commas, dots, k/M suffixes) immediately before the stat label.
frontend/__tests__/e2e/pages/ProjectDetails.spec.ts (1)
8-15: Extremely brittle: full paragraph description as exact text match.Lines 11–13 assert a ~500-character project description verbatim. Any minor edit to the OWASP Nest project description in the database will break this test. Consider using a partial regex match (like the
RepositoryDetailstest does with/Your gateway to OWASP/i) to verify the description is present without coupling to every word.Suggested partial match
await expect( - page.getByText( - 'OWASP Nest is a code project aimed at improving how OWASP manages its collection of projects. It will create a more organized system with better navigation and user experience while keeping the current risk profile intact. The project plans to use the Github API to keep track of project updates and automate certain tasks. Project leaders will be able to add important information using standardized files. The goal is to have a minimum viable product ready by September 2024, with hopes of completing it by the end of the year.' - ) + page.getByText(/OWASP Nest is a code project/i) ).toBeVisible()frontend/__tests__/e2e/pages/OrganizationDetails.spec.ts (1)
17-29: Section content locators are too broad and may match unrelated content.The
page.locator('div').filter({ hasText: 'Recent Issues' })pattern (and similar ones for Milestones, Releases, Pull Requests) matches any ancestordivcontaining that text — including the page root. The.locator('a').first()could then resolve to a link from an entirely different section, making the test pass vacuously.Consider scoping content checks to a container identified by the section heading, similar to how
[data-anchor-title="true"]is already used for presence checks:Example scoped approach for one section
test('should display recent issues section', async ({ page }) => { await expect( page.locator('[data-anchor-title="true"]', { hasText: 'Recent Issues' }) ).toBeVisible() - const firstIssue = page.locator('div').filter({ hasText: 'Recent Issues' }).locator('a').first() + const section = page.locator('section, div', { + has: page.getByRole('heading', { name: 'Recent Issues' }), + }) + const firstIssue = section.locator('a').first() await expect(firstIssue).toBeVisible() })The same applies to the milestones (lines 47–52), releases (lines 59–64), and pull requests (lines 79–84) tests.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
15-33: Same broad-locator concern as OrganizationDetails.The
page.locator('div').filter({ hasText: 'New Chapters' }).locator('a').first()pattern (lines 18–23, repeated for "New Projects" at 28–33) can match ancestor divs and resolve links from other sections. Consider scoping to a container identified by the heading, as suggested in the OrganizationDetails review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/run-ci-cd.yaml:
- Around line 368-375: In the "Run frontend end-to-end tests" step update the
NEXTAUTH_SECRET environment variable so it matches the frontend container's
secret; replace the hardcoded -e NEXTAUTH_SECRET=secret with -e
NEXTAUTH_SECRET=secret3234 to ensure token signature validation between the
frontend and the e2e test container uses the same NEXTAUTH_SECRET.
🧹 Nitpick comments (2)
.github/workflows/run-ci-cd.yaml (2)
339-347:-p 3000:3000is ignored with--network host.When
--network hostis used, port mappings have no effect (Docker emits a warning). The same applies to the backend container at Line 301. Consider removing-pfrom both to avoid confusion.Suggested fix
- name: Run frontend in the background run: | docker run -d --name e2e-nest-frontend \ --env-file frontend/.env.e2e.example \ --network host \ - -p 3000:3000 \ -e NEXT_SERVER_CSRF_URL=http://localhost:9000/csrf/ \ -e NEXT_SERVER_GRAPHQL_URL=http://localhost:9000/graphql/ \ owasp/nest:frontend-e2e-latest
359-366: No build cache configured for the e2e testing image.Unlike the a11y test image build (Line 394-398) and other image builds in this workflow, this step has neither
cache-fromnorcache-to. Playwright image builds can be expensive due to browser binary installation, so adding caching would speed up CI runs.Suggested fix
- name: Build e2e testing image uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 with: + cache-from: | + type=gha + type=registry,ref=owasp/nest:test-frontend-e2e-cache + cache-to: | + type=gha,compression=zstd context: frontend file: docker/frontend/Dockerfile.e2e.test
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
459-490:⚠️ Potential issue | 🟡 MinorClarify that the UI URL is config-dependent.
If the Playwright UI port changes in config or via env, the hardcoded
http://localhost:3800may become stale. Consider adding a brief note that the port comes from the Playwright config (or env) and may differ.
|
|
❌ The last analysis has failed. |
2 participants
Proposed change
Resolves #3030
Add the PR description here.
Checklist
make check-testlocally: all warnings addressed, tests passed