Add Playwright E2E test suite with CI integration#843
Merged
Conversation
Set up Playwright 1.58.2 with 50 E2E tests across 11 spec files that verify page rendering and role-based access control for all 5 access levels (unauthenticated, student, advisor, supervisor, admin). Tests authenticate through the Keycloak login form to establish proper SSO sessions and cover: dashboard, topics, applications, theses, interviews, presentations, settings, research groups, navigation, and authentication flows. Infrastructure: - playwright.config.ts with auth setup project and chromium - Auth setup via Keycloak login form (not direct access grant) - Reusable helpers for navigation and page load waiting - GitHub Actions reusable workflow (e2e_tests.yml) with PostgreSQL and Keycloak service containers - CI integration in dev.yml gating deployment on E2E tests - Idempotent local runner script (execute-e2e-local.sh) Documentation: - Developer docs in docs/DEVELOPMENT.md with execution instructions, test structure, role coverage, and coverage analysis table - CLAUDE.md updated with E2E test commands and coverage summary - README.md links to E2E test section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
WalkthroughAdds Playwright-based end-to-end testing: new CI workflow/job, standalone e2e workflow, Playwright config and npm scripts, many Playwright test suites and helpers, auth storage setup, local orchestration script, .gitignore updates, docs, and dev deployment now waits for E2E tests. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant PG as Postgres Service
participant KC as Keycloak
participant SV as Spring Boot Server
participant CL as Client Dev Server
participant PW as Playwright
CI->>PG: start Postgres service
CI->>KC: start Keycloak & import realm
CI->>SV: checkout & start server (dev profile) with DB/Keycloak env
CI->>CL: start client dev server
CI->>PW: run Playwright tests (pointing to client URL)
PW->>SV: poll server health
PW->>CL: poll client readiness
PW->>KC: perform user authentication as needed
PW->>CI: upload reports & test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
…tainer GitHub Actions service containers don't support custom commands, so the Keycloak image's default entrypoint just printed help and exited. Switch to running Keycloak via `docker run` with the `start-dev` command, and use the `/realms/master` endpoint for health checking (dev mode doesn't expose `/health/ready`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (13)
client/e2e/auth.setup.ts (1)
35-35: Consider usingauthStatePathhelper for path consistencyThe path is hardcoded inline here but
helpers.tsalready exportsauthStatePath(role)for exactly this purpose. Using it keeps all storage-state path logic in one place.♻️ Proposed refactor
+import { authStatePath } from './helpers' // ... - await page.context().storageState({ path: `e2e/.auth/${user.name}.json` }) + await page.context().storageState({ path: authStatePath(user.name) })Note:
user.namemust be narrowed to theauthStatePathunion type ('student' | 'advisor' | 'supervisor' | 'admin') —as constonTEST_USERSalready achieves this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/auth.setup.ts` at line 35, Replace the hardcoded storage-state path usage in the call to page.context().storageState({ path: `e2e/.auth/${user.name}.json` }) with the shared helper authStatePath(...) from helpers.ts to centralize path logic; call authStatePath(user.name) (or authStatePath(user.role) as appropriate) instead and ensure the value passed matches the helper's union type (narrow TEST_USERS with as const so user.name conforms to 'student'|'advisor'|'supervisor'|'admin'). This keeps path formatting consistent and avoids duplicated string logic.client/e2e/theses.spec.ts (1)
9-11: Broad/create/iselector risks strict-mode failures if multiple matches exist.
getByRole('button', { name: /create/i })(without.first()) runs in strict mode and will throw if more than one button matches. The student test (toBeHidden()) depends on exactly one match. Consider a more specific name like/create thesis/ito be safe.💡 Suggested change
- await expect(page.getByRole('button', { name: /create/i })).toBeHidden() + await expect(page.getByRole('button', { name: /create thesis/i })).toBeHidden()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/theses.spec.ts` around lines 9 - 11, The selector for the student "create" button is too broad and will fail in strict mode if multiple buttons match; update the call to page.getByRole('button', { name: /create/i }) used with toBeHidden() in the test (theses.spec.ts) to target a unique control (for example change the name regex to /create thesis/i or otherwise narrow the accessible name) or explicitly select the intended element (e.g., use .first() only if that is guaranteed) so the assertion await expect(...).toBeHidden() reliably matches a single button.client/playwright.config.ts (2)
7-7: Consider setting local retries to0.
retries: 1locally means a flaky test that fails on the first run re-executes and silently passes. Setting retries to0locally surfaces flakiness earlier; keep2only for CI.♻️ Suggested change
- retries: process.env.CI ? 2 : 1, + retries: process.env.CI ? 2 : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/playwright.config.ts` at line 7, Update the Playwright config's retries setting so local runs don't auto-retry flaky tests: change the retries expression in the exported config (the retries property in playwright.config.ts) to use 0 for non-CI environments, e.g. replace "retries: process.env.CI ? 2 : 1" with "retries: process.env.CI ? 2 : 0" so only CI keeps multiple retries.
22-29: Implicit student session is a hidden default for every test.Any spec file that omits
test.use({ storageState: ... })silently runs authenticated as a student. Consider documenting this contract clearly (e.g., a JSDoc comment above the project) so future test authors don't accidentally inherit a role they didn't intend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/playwright.config.ts` around lines 22 - 29, Add a short JSDoc comment immediately above the Playwright project object named 'chromium' that documents the implicit authenticated session created by the project's storageState (storageState: 'e2e/.auth/student.json'), explaining that any spec that does not call test.use({ storageState: ... }) will silently run as the student and instructing authors to explicitly set or override storageState in their tests; optionally note the project name or consider making this an opt-in project to avoid accidental inheritance.client/e2e/research-groups.spec.ts (1)
20-28: Search test only asserts the positive case; filtering is not actually verified.After typing "Applied Software", only
Applied Software Engineeringbeing visible is checked.Data Science and Analyticscould still be visible (i.e., the filter may be broken), and the test would still pass. Add a negative assertion.💡 Suggested addition
await expect(page.getByText('Applied Software Engineering').first()).toBeVisible() + // Filtered-out group should no longer appear + await expect(page.getByText('Data Science and Analytics')).toBeHidden()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/research-groups.spec.ts` around lines 20 - 28, The test "search filters research groups" currently only asserts the positive match; after filling the search input via page.getByPlaceholder(/search research groups/i).fill('Applied Software') add a negative assertion to verify non-matching groups are hidden — e.g., use page.getByText('Data Science and Analytics') (or a selector for another known non-matching group) and assert .not.toBeVisible() or .toHaveCount(0) so the filter truly excludes unrelated entries while keeping Applied Software Engineering visible via page.getByText('Applied Software Engineering').first().client/e2e/auth.spec.ts (1)
59-95: Advisor and Supervisor test blocks have identical assertions — consider parameterizing.Both describe blocks at Lines 59 and 78 make the exact same set of visibility/hiddenness assertions. Any future permission change that affects only one role won't be caught because both pass. A parameterized approach makes the shared expectation explicit and future divergence immediately testable.
♻️ Suggested refactor
-test.describe('Authentication - Advisor', () => { - test.use({ storageState: authStatePath('advisor') }) - - test('sees management nav items but not admin items', async ({ page }) => { - await navigateTo(page, '/dashboard') - await expect(page.getByRole('heading', { name: /dashboard/i })).toBeVisible({ timeout: 15_000 }) - await expect(page.getByRole('link', { name: 'Review Applications' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Manage Topics' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Theses Overview' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Interviews' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Submit Application' })).toBeHidden() - await expect(page.getByRole('link', { name: 'Research Groups' })).toBeHidden() - }) -}) - -test.describe('Authentication - Supervisor', () => { - test.use({ storageState: authStatePath('supervisor') }) - - test('sees management nav items but not admin items', async ({ page }) => { - await navigateTo(page, '/dashboard') - await expect(page.getByRole('heading', { name: /dashboard/i })).toBeVisible({ timeout: 15_000 }) - await expect(page.getByRole('link', { name: 'Review Applications' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Manage Topics' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Theses Overview' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Interviews' })).toBeVisible() - await expect(page.getByRole('link', { name: 'Submit Application' })).toBeHidden() - await expect(page.getByRole('link', { name: 'Research Groups' })).toBeHidden() - }) -}) +for (const role of ['advisor', 'supervisor'] as const) { + test.describe(`Authentication - ${role}`, () => { + test.use({ storageState: authStatePath(role) }) + + test('sees management nav items but not admin items', async ({ page }) => { + await navigateTo(page, '/dashboard') + await expect(page.getByRole('heading', { name: /dashboard/i })).toBeVisible({ timeout: 15_000 }) + await expect(page.getByRole('link', { name: 'Review Applications' })).toBeVisible() + await expect(page.getByRole('link', { name: 'Manage Topics' })).toBeVisible() + await expect(page.getByRole('link', { name: 'Theses Overview' })).toBeVisible() + await expect(page.getByRole('link', { name: 'Interviews' })).toBeVisible() + await expect(page.getByRole('link', { name: 'Submit Application' })).toBeHidden() + await expect(page.getByRole('link', { name: 'Research Groups' })).toBeHidden() + }) + }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/auth.spec.ts` around lines 59 - 95, The two test.describe blocks duplicate the same assertions for Advisor and Supervisor; refactor by parameterizing the role and expectations into a single reusable test function (e.g., create a helper like runRoleNavTests(role: string) or table-driven loop) that calls test.describe/test for each role using test.use({ storageState: authStatePath(role) }), navigates with navigateTo(page, '/dashboard'), and runs the existing expects via page.getByRole; update/remove the duplicated test.describe blocks and invoke the helper for 'advisor' and 'supervisor' so future role-specific differences are easy to add and maintain.client/e2e/navigation.spec.ts (1)
56-81:Navigation - Student routessilently relies on the global default session.Unlike every other role-specific block, this describe group has no
test.use({ storageState: ... }). It works because the chromium project defaults tostudent.json, but the intent isn't locally obvious. Adding an explicit annotation mirrors the pattern used by supervisor/admin blocks and makes the role contract self-documenting.💡 Suggested addition
test.describe('Navigation - Student routes', () => { + test.use({ storageState: authStatePath('student') }) + test('can navigate between pages via sidebar', async ({ page }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/navigation.spec.ts` around lines 56 - 81, The describe block "Navigation - Student routes" currently relies on the global default session; make the role explicit by adding a per-describe test fixture like test.use({ storageState: 'student.json' }) at the top of the test.describe block so both tests inside (the "can navigate between pages via sidebar" and "header logo navigates to dashboard when authenticated") run with the student authenticated session; place this near the describe declaration to mirror the supervisor/admin pattern and ensure the role contract is self-documenting.client/e2e/applications.spec.ts (2)
20-26: Duplicate of the student dashboard test indashboard.spec.ts.This test navigates to
/dashboardand asserts "My Applications" heading — exactly whatdashboard.spec.tsline 5–13 already covers. Either remove it here or differentiate the assertion (e.g., verify application count or a specific application entry).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/applications.spec.ts` around lines 20 - 26, This test "dashboard shows student applications section" duplicates coverage from dashboard.spec.ts; remove this test from client/e2e/applications.spec.ts or change it to a distinct assertion: keep the test named dashboard shows student applications section only if you add a new expectation (e.g., after navigateTo(page, '/dashboard') assert a concrete application detail or count using page.getByRole('heading', { name: /my applications/i }) plus checks for a specific application entry or application count element), otherwise delete the entire test block to avoid duplication with dashboard.spec.ts.
10-10: Fragile CSS class selector.mantine-Stepper-root.Mantine's internal class names can change across versions. Prefer a role-based locator or add a
data-testidto the stepper component for stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/applications.spec.ts` at line 10, The test uses a fragile CSS selector '.mantine-Stepper-root' in the locator passed to expect(...).toBeVisible; replace this with a stable locator by either adding a data-testid attribute to the Stepper component (e.g., data-testid="app-stepper") and updating the test to use page.getByTestId('app-stepper') or use a semantic/role-based locator (e.g., page.getByRole('progressbar' or 'tablist') depending on the Stepper's ARIA) instead of '.mantine-Stepper-root' so the assertion (the call to toBeVisible) targets a stable selector.client/e2e/topics.spec.ts (1)
18-21: No assertion that the view actually changed after tab clicks.Clicking "Published Topics" and "Open Topics" only verifies no crash occurs. Consider asserting a visible change (e.g., different topic entries, a tab active state, or a URL parameter change) to make the test meaningful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/topics.spec.ts` around lines 18 - 21, After clicking the tabs with page.getByText('Published Topics').click() and page.getByText('Open Topics').click(), add an assertion that the view actually changed: for example assert the clicked tab shows an active state (aria-selected/active class) or assert the topic list contents differ (check for a known published topic when Published is clicked and a different open topic when Open is clicked) or assert a URL/query param change; update the test around the two page.getByText(...) calls to include one of these concrete checks so the test verifies behavior rather than just absence of a crash.client/e2e/dashboard.spec.ts (1)
27-36: Supervisor test is identical to the Advisor test — no "management view" assertion.The test name claims to verify "management view" but only checks the same two headings as the Advisor test (Dashboard + My Theses). Consider asserting at least one element unique to the Supervisor/Examiner dashboard (e.g., a management-specific section or action button) to justify the distinct test name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/dashboard.spec.ts` around lines 27 - 36, The "Dashboard - Supervisor" test (test.describe block using test.use with authStatePath('supervisor') and the test named 'shows dashboard with management view') currently only asserts the generic headings checked by the Advisor test; update the test to assert at least one supervisor-specific UI element after navigateTo(page, '/dashboard'), e.g., expect a management-specific heading, section, or action button (use page.getByRole/getByText for a 'Management', 'Manage Theses', or 'Assign Examiner' label) toBeVisible so the test actually verifies the management view unique to supervisors.execute-e2e-local.sh (1)
34-36:lsofdependency — minor portability note.
lsofis standard on macOS but may not be installed on minimal Linux environments. Since this is a local dev script, it's fine — just worth a note in the script header or docs if targeting diverse dev setups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execute-e2e-local.sh` around lines 34 - 36, The is_port_open() function relies on the lsof binary which may be missing on minimal Linux systems; update the script to either document this dependency in the script header (and README) or add a runtime check (e.g., use command -v lsof) and a fallback to ss or netstat if lsof is not present so the function still works across macOS and minimal Linux environments..github/workflows/e2e_tests.yml (1)
86-99: Backgrounded server with no failure detection before health check.If
gradlew bootRunexits immediately (e.g., compilation error), the workflow will wait the full 120 iterations (4 min) before failing at the health check. Consider capturing the background PID and checking it's still alive in the wait loop.Also:
KEYCLOAK_SERVICE_CLIENT_SECRET: ""— empty string is fine for dev/test but ensure the server doesn't reject it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e_tests.yml around lines 86 - 99, The "Start server" step runs ./gradlew bootRun in background without checking it started; modify that step to capture the background PID of the gradlew process (e.g., after launching bootRun save $! to a file or env var) and in the existing health-check/wait loop verify the PID is still alive (use kill -0 or ps) on each iteration and fail immediately if the process has exited, so the workflow doesn't wait the full timeout when bootRun crashes; also replace KEYCLOAK_SERVICE_CLIENT_SECRET: "" with a non-empty dev placeholder or add a comment/env check to ensure the server accepts an empty secret (update the Start server step and the wait/health-check logic that polls the app).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e_tests.yml:
- Around line 49-58: The Keycloak readiness loop in the "Wait for Keycloak to be
ready" job silently continues on timeout and leaves the loop variable unused;
update the loop so that if curl never succeeds after the attempts it prints a
clear timeout error and exits non‑zero (exit 1) to fail the workflow, and
address SC2034 by replacing or using the loop variable (e.g., use _ or reference
i in the timeout message). Ensure the readiness check still uses the same health
URL (http://localhost:8081/health/ready) and that the logic breaks on success
and exits with a failure and message on timeout.
- Around line 128-156: Replace unquoted shell variable comparisons in the two
GitHub Actions run steps ("Wait for server to be ready" and "Wait for client to
be ready") by quoting $i in the arithmetic test expressions; update the
conditions that currently read [ $i -eq 120 ] and [ $i -eq 60 ] to use "[ \"$i\"
-eq 120 ]" and "[ \"$i\" -eq 60 ]" respectively so the SC2086 warning is
resolved and the tests are robust against word-splitting or empty values.
In `@CLAUDE.md`:
- Line 26: Update the deprecated CLI invocation in CLAUDE.md by replacing the
standalone "docker-compose up" command with the Docker CLI plugin form "docker
compose up" so the README uses the V2-consistent command (search for the string
"docker-compose up" in CLAUDE.md and change it to "docker compose up").
In `@client/e2e/applications.spec.ts`:
- Around line 32-38: The current test 'review page loads with application
sidebar' uses a weak assertion page.locator('body').toContainText(/.+/) which is
a no-op; replace it with a meaningful check that verifies the applications page
rendered by asserting a specific element or text such as the page heading, the
sidebar container, or the empty-state message returned by the applications
route—locate and assert an element like the applications sidebar selector (e.g.,
a CSS class or data-testid for the sidebar), the applications page heading text,
or the known empty-state message after calling navigateTo(page, '/applications')
and before/after the expect(page).toHaveURL check to ensure the correct content
is present.
In `@client/e2e/auth.setup.ts`:
- Around line 27-32: The inline function passed to page.waitForFunction
currently calls JSON.parse on localStorage.getItem('authentication_tokens')
which can throw and abort retries; update the function used in
page.waitForFunction (the anonymous callback passed to page.waitForFunction) to
defensively handle missing or malformed data by wrapping JSON.parse in a
try/catch (or otherwise validating the string) and returning false on parse
failure, and only returning true when both parsed.access_token and
parsed.refresh_token are present so the wait can retry until timeout instead of
failing immediately.
In `@client/e2e/interviews.spec.ts`:
- Line 18: The test uses a hardcoded seeded interview UUID in the navigateTo
call (navigateTo(page, '/interviews/00000000-0000-4000-e600-000000000001'))
which couples the spec to the seed data; add an inline comment immediately above
that call explaining that this UUID comes from the seed script and is expected
to be stable because the seed uses ON CONFLICT DO NOTHING, so future maintainers
understand the dependency (or note that the test should be updated if seed PKs
change).
- Around line 37-44: The "Interviews - Student cannot access" describe/test is
missing authentication state so it runs as unauthenticated; update the describe
block (the block with title 'Interviews - Student cannot access') to run with a
student storage state by adding test.use({ storageState:
authStatePath('student') }) so the test exercises logged-in student RBAC (keep
the existing navigateTo and expectation intact); ensure you reference
authStatePath('student') so the browser context is seeded as a student before
calling navigateTo(page, '/interviews').
In `@client/e2e/presentations.spec.ts`:
- Around line 5-9: The test "presentations page shows heading and calendar" only
asserts the heading; update the test in presentations.spec.ts to either (A) add
a calendar assertion after navigateTo — e.g., use page.getByRole or page.locator
to assert the calendar element is visible/contains expected dates/events (refer
to the existing test function and page object usage like navigateTo and
page.getByRole), or (B) change the test name to only mention the heading; ensure
the assertion and test title match (modify the test(...) title or add a second
await expect(...) for the calendar element such as page.getByRole('region', {
name: 'Calendar' }) or a selector used by your app).
In `@client/e2e/research-groups.spec.ts`:
- Around line 34-41: The test "supervisor can access their research group
settings" uses a vacuous assertion
expect(page.locator('body')).toContainText(/.+/) that doesn't verify the
settings page; replace it with a targeted assertion that checks for a specific,
unique page element or text (e.g., the research group name, settings page
heading, or an explicit "Access denied" message) after calling navigateTo(page,
'/research-groups/00000000-0000-4000-a000-000000000001'); update the expectation
to assert the exact heading/group name text or the presence/visibility of a
settings form element (use the same page.locator(...) pattern to reference that
selector) so the test fails when the wrong page (404/unauthorized) is shown.
In `@client/e2e/theses.spec.ts`:
- Around line 48-52: Remove the duplicate comment line "// Key thesis sections
should be visible (accordion sections)" that appears twice in a row above the
assertions; keep a single instance of that comment near the assertions (the
lines containing await expect(page.getByText('Configuration')).toBeVisible() and
await expect(page.getByText('Involved Persons')).toBeVisible()) so the test is
no-op behavior unchanged but without redundant comments.
In `@execute-e2e-local.sh`:
- Around line 132-134: The client start uses a subshell so save_pid captures the
subshell PID, not the webpack dev server; change the invocation to use exec
inside the subshell so the shell process is replaced by the webpack process
(e.g. (cd "$CLIENT_DIR" && exec npx webpack serve --env NODE_ENV=development >
"$ROOT_DIR/.e2e-client.log" 2>&1) &), then call save_pid "client" $! so kill_pid
"client" will target the actual webpack process; update the start line where
CLIENT_DIR, save_pid, and the npx webpack serve invocation are located
accordingly.
- Around line 119-121: The subshell launch "(cd \"$SERVER_DIR\" && ./gradlew
bootRun ... ) &" causes $! to capture the subshell PID, risking orphaned
Java/Gradle processes; change the start so the actual Gradle/Java process is the
background job (e.g., cd into SERVER_DIR in the current shell then start
./gradlew bootRun & and call save_pid "server" $!), or explicitly create a new
process group and save that group leader so kill_pid can kill the group (e.g.,
start with setsid or use "exec" so the child becomes the job); update the code
around the server-starting block and ensure kill_pid uses kill -- -$PID to
terminate the process group if you choose the group approach, referencing the
save_pid and kill_pid functions and the SERVER_DIR / ROOT_DIR start block.
---
Nitpick comments:
In @.github/workflows/e2e_tests.yml:
- Around line 86-99: The "Start server" step runs ./gradlew bootRun in
background without checking it started; modify that step to capture the
background PID of the gradlew process (e.g., after launching bootRun save $! to
a file or env var) and in the existing health-check/wait loop verify the PID is
still alive (use kill -0 or ps) on each iteration and fail immediately if the
process has exited, so the workflow doesn't wait the full timeout when bootRun
crashes; also replace KEYCLOAK_SERVICE_CLIENT_SECRET: "" with a non-empty dev
placeholder or add a comment/env check to ensure the server accepts an empty
secret (update the Start server step and the wait/health-check logic that polls
the app).
In `@client/e2e/applications.spec.ts`:
- Around line 20-26: This test "dashboard shows student applications section"
duplicates coverage from dashboard.spec.ts; remove this test from
client/e2e/applications.spec.ts or change it to a distinct assertion: keep the
test named dashboard shows student applications section only if you add a new
expectation (e.g., after navigateTo(page, '/dashboard') assert a concrete
application detail or count using page.getByRole('heading', { name: /my
applications/i }) plus checks for a specific application entry or application
count element), otherwise delete the entire test block to avoid duplication with
dashboard.spec.ts.
- Line 10: The test uses a fragile CSS selector '.mantine-Stepper-root' in the
locator passed to expect(...).toBeVisible; replace this with a stable locator by
either adding a data-testid attribute to the Stepper component (e.g.,
data-testid="app-stepper") and updating the test to use
page.getByTestId('app-stepper') or use a semantic/role-based locator (e.g.,
page.getByRole('progressbar' or 'tablist') depending on the Stepper's ARIA)
instead of '.mantine-Stepper-root' so the assertion (the call to toBeVisible)
targets a stable selector.
In `@client/e2e/auth.setup.ts`:
- Line 35: Replace the hardcoded storage-state path usage in the call to
page.context().storageState({ path: `e2e/.auth/${user.name}.json` }) with the
shared helper authStatePath(...) from helpers.ts to centralize path logic; call
authStatePath(user.name) (or authStatePath(user.role) as appropriate) instead
and ensure the value passed matches the helper's union type (narrow TEST_USERS
with as const so user.name conforms to
'student'|'advisor'|'supervisor'|'admin'). This keeps path formatting consistent
and avoids duplicated string logic.
In `@client/e2e/auth.spec.ts`:
- Around line 59-95: The two test.describe blocks duplicate the same assertions
for Advisor and Supervisor; refactor by parameterizing the role and expectations
into a single reusable test function (e.g., create a helper like
runRoleNavTests(role: string) or table-driven loop) that calls
test.describe/test for each role using test.use({ storageState:
authStatePath(role) }), navigates with navigateTo(page, '/dashboard'), and runs
the existing expects via page.getByRole; update/remove the duplicated
test.describe blocks and invoke the helper for 'advisor' and 'supervisor' so
future role-specific differences are easy to add and maintain.
In `@client/e2e/dashboard.spec.ts`:
- Around line 27-36: The "Dashboard - Supervisor" test (test.describe block
using test.use with authStatePath('supervisor') and the test named 'shows
dashboard with management view') currently only asserts the generic headings
checked by the Advisor test; update the test to assert at least one
supervisor-specific UI element after navigateTo(page, '/dashboard'), e.g.,
expect a management-specific heading, section, or action button (use
page.getByRole/getByText for a 'Management', 'Manage Theses', or 'Assign
Examiner' label) toBeVisible so the test actually verifies the management view
unique to supervisors.
In `@client/e2e/navigation.spec.ts`:
- Around line 56-81: The describe block "Navigation - Student routes" currently
relies on the global default session; make the role explicit by adding a
per-describe test fixture like test.use({ storageState: 'student.json' }) at the
top of the test.describe block so both tests inside (the "can navigate between
pages via sidebar" and "header logo navigates to dashboard when authenticated")
run with the student authenticated session; place this near the describe
declaration to mirror the supervisor/admin pattern and ensure the role contract
is self-documenting.
In `@client/e2e/research-groups.spec.ts`:
- Around line 20-28: The test "search filters research groups" currently only
asserts the positive match; after filling the search input via
page.getByPlaceholder(/search research groups/i).fill('Applied Software') add a
negative assertion to verify non-matching groups are hidden — e.g., use
page.getByText('Data Science and Analytics') (or a selector for another known
non-matching group) and assert .not.toBeVisible() or .toHaveCount(0) so the
filter truly excludes unrelated entries while keeping Applied Software
Engineering visible via page.getByText('Applied Software Engineering').first().
In `@client/e2e/theses.spec.ts`:
- Around line 9-11: The selector for the student "create" button is too broad
and will fail in strict mode if multiple buttons match; update the call to
page.getByRole('button', { name: /create/i }) used with toBeHidden() in the test
(theses.spec.ts) to target a unique control (for example change the name regex
to /create thesis/i or otherwise narrow the accessible name) or explicitly
select the intended element (e.g., use .first() only if that is guaranteed) so
the assertion await expect(...).toBeHidden() reliably matches a single button.
In `@client/e2e/topics.spec.ts`:
- Around line 18-21: After clicking the tabs with page.getByText('Published
Topics').click() and page.getByText('Open Topics').click(), add an assertion
that the view actually changed: for example assert the clicked tab shows an
active state (aria-selected/active class) or assert the topic list contents
differ (check for a known published topic when Published is clicked and a
different open topic when Open is clicked) or assert a URL/query param change;
update the test around the two page.getByText(...) calls to include one of these
concrete checks so the test verifies behavior rather than just absence of a
crash.
In `@client/playwright.config.ts`:
- Line 7: Update the Playwright config's retries setting so local runs don't
auto-retry flaky tests: change the retries expression in the exported config
(the retries property in playwright.config.ts) to use 0 for non-CI environments,
e.g. replace "retries: process.env.CI ? 2 : 1" with "retries: process.env.CI ? 2
: 0" so only CI keeps multiple retries.
- Around line 22-29: Add a short JSDoc comment immediately above the Playwright
project object named 'chromium' that documents the implicit authenticated
session created by the project's storageState (storageState:
'e2e/.auth/student.json'), explaining that any spec that does not call
test.use({ storageState: ... }) will silently run as the student and instructing
authors to explicitly set or override storageState in their tests; optionally
note the project name or consider making this an opt-in project to avoid
accidental inheritance.
In `@execute-e2e-local.sh`:
- Around line 34-36: The is_port_open() function relies on the lsof binary which
may be missing on minimal Linux systems; update the script to either document
this dependency in the script header (and README) or add a runtime check (e.g.,
use command -v lsof) and a fallback to ss or netstat if lsof is not present so
the function still works across macOS and minimal Linux environments.
The lock file had stale @tiptap transitive dependencies (core@3.16.0) that don't satisfy the requirements of newer tiptap packages (3.20.0). Regenerated from scratch to resolve all version conflicts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix searchAndSelectMultiSelect helper to properly select options in Mantine UserMultiSelect by dispatching full mouse event chain via evaluate when Playwright's built-in click doesn't trigger React handlers in portal-rendered combobox dropdowns - Make application-workflow test resilient to test data from prior runs (handle pre-selected topics, already-uploaded files, duplicate submissions) - Fix proposal-feedback test strict mode violation with .first() - Verify topic creation via success notification instead of table text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
client/e2e/helpers.ts (3)
38-43:fillRichTextEditor— usespage.keyboard.type()which types globally, not scoped to the editor.After
editor.click(), focus should be on the ProseMirror element, so this works in practice. But if any intermediate UI element steals focus (e.g., a tooltip), keystrokes go elsewhere silently. Usingeditor.type(text)(Playwright's locator-scoped typing) would be safer, though note thatLocator.type()is deprecated in favor oflocator.pressSequentially().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/helpers.ts` around lines 38 - 43, The helper fillRichTextEditor currently uses page.keyboard.type() which sends keystrokes globally; change it to perform typing on the ProseMirror locator itself (the editor locator obtained from wrapper.locator('.ProseMirror')) so input is scoped to the editor—use the locator-scoped API (prefer locator.pressSequentially(...) if your Playwright version deprecates locator.type) to send the text to the editor locator after clicking it.
82-82:waitForTimeout(500)is a hard-coded sleep — acceptable here given the portal dropdown workaround, but keep it minimal.Consider replacing with a condition-based wait if flakiness persists. The comment on the fallback approach is appreciated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/helpers.ts` at line 82, The hard-coded sleep usage (page.waitForTimeout) in client/e2e/helpers.ts is acceptable as a fallback but should be minimized or replaced; reduce the timeout to the smallest reliable value (e.g., ~200ms) and, ideally, replace the sleep with a condition-based wait that targets the portal dropdown (use page.waitForSelector or page.waitForFunction for the specific dropdown selector/state) so the test waits for the UI condition instead of a fixed delay; update the code where page.waitForTimeout(500) is used and add a short comment noting the fallback for the portal dropdown workaround.
7-16:waitForPageLoad— silently swallowing all errors from.catch(() => {}).This catches any error, not just "loader never appeared." If the locator itself is broken (e.g., class name changes), you'll get silent green tests with pages that aren't actually loaded. Consider logging or at least narrowing the catch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/helpers.ts` around lines 7 - 16, The waitForPageLoad helper silently swallows all errors from the locator.waitFor call (function waitForPageLoad, locator '.mantine-Loader-root'), which can hide real failures; update the catch to only ignore Playwright timeout errors (e.g., check error.name === 'TimeoutError') or explicitly detect the timeout condition, and for any other error either rethrow or log a warning with the error details so broken locators surface instead of producing silent green tests.client/e2e/application-workflow.spec.ts (1)
98-107: Hard sleep + weak fallback assertion after submission.
waitForTimeout(2_000)followed by an "is still on the same page" fallback means a server 500 that re-renders the form will pass this test. Replace with:♻️ Proposed improvement
- // Wait for the submission to process - await page.waitForTimeout(2_000) - // Verify success (may already be submitted from a prior run, check for either outcome) - const successMessage = page.getByText('Your application was successfully submitted!') - const isSuccess = await successMessage.isVisible().catch(() => false) - if (!isSuccess) { - // If not showing success, verify we at least made it through the full stepper - // by checking we're still on the submit application page (no crash/error page) - await expect(page).toHaveURL(/\/submit-application/) - } + // Verify success or that we hit the duplicate-submission guard + await expect( + page.getByText('Your application was successfully submitted!') + .or(page.getByText('already submitted')) + ).toBeVisible({ timeout: 15_000 })Using Playwright's
.or()combinator lets you wait for either outcome without a sleep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/application-workflow.spec.ts` around lines 98 - 107, Replace the hard sleep and weak fallback with a deterministic wait for either the success message or the submit page URL: remove page.waitForTimeout and the isSuccess check and instead await Promise.race between successMessage.waitFor({ state: 'visible' }) and page.waitForURL(/\/submit-application/); locate the existing symbols successMessage (from page.getByText('Your application was successfully submitted!')) and the expect(page).toHaveURL fallback to implement the race so the test reliably waits for either outcome without sleeping.client/e2e/presentation-workflow.spec.ts (1)
71-73: Fragile selector:locator('button').last()for the date-picker confirm button.If the date-picker UI adds any button after the confirm action, this breaks. Prefer a more specific selector (e.g., by
aria-label, icon, or test ID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/presentation-workflow.spec.ts` around lines 71 - 73, The selector datePickerDialog.locator('button').last() is fragile; replace it with a specific selector that targets the confirm/checkmark action (e.g., use an aria-label or accessible name, test id, or the icon role) so the test won’t break if other buttons are added. Update the click call on the datePickerDialog element (the line using datePickerDialog.locator('button').last().click()) to use something like locator('button[aria-label="Confirm"]'), getByRole('button', { name: /confirm|check/i }), or a dedicated data-testid for the confirm button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/e2e/application-workflow.spec.ts`:
- Around line 57-61: The test currently uses an unscoped
page.getByRole('checkbox') which will match the first checkbox on the page
(privacyCheckbox) and can break if another checkbox is added; update the
selector to scope by its visible label (e.g., use page.getByRole('checkbox', {
name: /privacy notice|accept privacy/i }) or page.getByLabelText equivalent) so
privacyCheckbox targets the actual privacy consent checkbox before calling
isChecked()/check().
In `@client/e2e/helpers.ts`:
- Around line 69-100: The helper searchAndSelectMultiSelect currently accepts
_searchText but never types it, so server-side filtered options may never load;
update the function to type the provided _searchText into the textbox (use
textbox.fill or textbox.type with a small delay) before querying listbox/option
and before the fallback retry (i.e., before locating option and retryOption),
then wait for the expected option to appear (expect(...).toBeVisible) or add a
short waitForResponse/timeout as needed to allow server fetch to complete; keep
the existing option selection logic (option/retryOption evaluate) but ensure
_searchText is actually sent to the input so server-side search is triggered.
In `@client/e2e/presentation-workflow.spec.ts`:
- Line 61: The test uses a hardcoded date in the selector
(datePickerDialog.getByRole('button', { name: /25 February 2026/i })). Replace
that literal with a computed date string so the test stays stable: compute a
target Date (e.g., today plus a safe offset or the 15th of next month), format
it to match the picker label format (e.g., "d MMMM yyyy"), and use that
formatted string (or a RegExp built from it) in the call to
datePickerDialog.getByRole(...). Ensure the same selector
(datePickerDialog.getByRole) is used but supplied with the dynamically formatted
name value so month-navigation is added if needed before clicking.
---
Duplicate comments:
In `@client/e2e/auth.setup.ts`:
- Around line 29-34: Wrap the JSON.parse inside the callback passed to
page.waitForFunction in a try/catch so a malformed authentication_tokens value
returns false instead of throwing; specifically update the anonymous function
that reads localStorage.getItem('authentication_tokens') used in
page.waitForFunction to catch JSON.parse errors and return false on exception,
allowing Playwright to keep retrying until the timeout.
---
Nitpick comments:
In `@client/e2e/application-workflow.spec.ts`:
- Around line 98-107: Replace the hard sleep and weak fallback with a
deterministic wait for either the success message or the submit page URL: remove
page.waitForTimeout and the isSuccess check and instead await Promise.race
between successMessage.waitFor({ state: 'visible' }) and
page.waitForURL(/\/submit-application/); locate the existing symbols
successMessage (from page.getByText('Your application was successfully
submitted!')) and the expect(page).toHaveURL fallback to implement the race so
the test reliably waits for either outcome without sleeping.
In `@client/e2e/helpers.ts`:
- Around line 38-43: The helper fillRichTextEditor currently uses
page.keyboard.type() which sends keystrokes globally; change it to perform
typing on the ProseMirror locator itself (the editor locator obtained from
wrapper.locator('.ProseMirror')) so input is scoped to the editor—use the
locator-scoped API (prefer locator.pressSequentially(...) if your Playwright
version deprecates locator.type) to send the text to the editor locator after
clicking it.
- Line 82: The hard-coded sleep usage (page.waitForTimeout) in
client/e2e/helpers.ts is acceptable as a fallback but should be minimized or
replaced; reduce the timeout to the smallest reliable value (e.g., ~200ms) and,
ideally, replace the sleep with a condition-based wait that targets the portal
dropdown (use page.waitForSelector or page.waitForFunction for the specific
dropdown selector/state) so the test waits for the UI condition instead of a
fixed delay; update the code where page.waitForTimeout(500) is used and add a
short comment noting the fallback for the portal dropdown workaround.
- Around line 7-16: The waitForPageLoad helper silently swallows all errors from
the locator.waitFor call (function waitForPageLoad, locator
'.mantine-Loader-root'), which can hide real failures; update the catch to only
ignore Playwright timeout errors (e.g., check error.name === 'TimeoutError') or
explicitly detect the timeout condition, and for any other error either rethrow
or log a warning with the error details so broken locators surface instead of
producing silent green tests.
In `@client/e2e/presentation-workflow.spec.ts`:
- Around line 71-73: The selector datePickerDialog.locator('button').last() is
fragile; replace it with a specific selector that targets the confirm/checkmark
action (e.g., use an aria-label or accessible name, test id, or the icon role)
so the test won’t break if other buttons are added. Update the click call on the
datePickerDialog element (the line using
datePickerDialog.locator('button').last().click()) to use something like
locator('button[aria-label="Confirm"]'), getByRole('button', { name:
/confirm|check/i }), or a dedicated data-testid for the confirm button.
- Fix shell quoting in CI workflow (quote $i in arithmetic comparisons) - Wrap JSON.parse in try/catch in auth setup to handle malformed data - Add missing storageState for student interview access test - Replace vacuous body assertions with meaningful element checks - Fix test description (presentations heading, not calendar) - Remove duplicate comment in theses.spec.ts - Add seed UUID documentation comments - Use exec in subshells to fix PID tracking in local runner - Simplify CLAUDE.md to essential build/test commands - Make application-workflow resilient to all prior-run states - Increase loader wait timeout to 30s for slow pages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ckbox, fix hardcoded date - Remove unused _searchText parameter from searchAndSelectMultiSelect helper and all callers - Scope checkbox selector in application-workflow to avoid matching multiple checkboxes - Replace hardcoded date with dynamic last-day-of-month selection in presentation-workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Document the 5 new workflow tests (topic, thesis, application, presentation, proposal feedback) in the test structure table and update the coverage matrix to reflect the expanded end-to-end workflow coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
client/e2e/presentations.spec.ts (1)
25-31: Hardcoded seed UUID with no comment — inconsistent withinterviews.spec.ts.
interviews.spec.ts(Line 17) now has a seed-coupling comment; add one here for consistency:📝 Suggested comment
+ // UUID matches the public presentation seeded in seed_dev_test_data.sql (thesis 3, final, public visibility) await navigateTo(page, '/presentations/00000000-0000-4000-e300-000000000002')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/presentations.spec.ts` around lines 25 - 31, The test 'presentation detail page is accessible without login' uses a hardcoded seed UUID in the navigateTo call (navigateTo(page, '/presentations/00000000-0000-4000-e300-000000000002')) but lacks the explanatory seed-coupling comment present in interviews.spec.ts; add a short comment above that line stating this UUID is a seeded public presentation (thesis 3, final, public visibility) and that the test relies on the seeded DB so future maintainers know the dependency.client/e2e/research-groups.spec.ts (1)
20-28: Search filter test only asserts the matching group remains — doesn't verify non-matching group is hidden.Adding the negative assertion would make filter behavior actually validated:
💡 Optional improvement
await page.getByPlaceholder(/search research groups/i).fill('Applied Software') // ASE should still be visible await expect(page.getByText('Applied Software Engineering').first()).toBeVisible() + // DSA should be filtered out + await expect(page.getByText('Data Science and Analytics').first()).toBeHidden()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/research-groups.spec.ts` around lines 20 - 28, The test "search filters research groups" currently only asserts that the matching group remains visible; add a negative assertion to verify non-matching groups are hidden after filling the search. After calling navigateTo and filling the placeholder via page.getByPlaceholder(/search research groups/i).fill('Applied Software'), add an expectation using page.getByText(...) for a known non-matching group (or use a selector that matches all groups) and assert .not.toBeVisible() or .toHaveCount(0). Keep the existing assertions (heading visible and matching group visible) and place the negative assertion right after the positive check so the test validates both presence and absence.execute-e2e-local.sh (1)
34-36:lsofmay be absent on minimal Linux environments.On some Debian/Ubuntu setups
lsofisn't pre-installed. Consider a more portable port-check alternative:♻️ Portable alternative
-is_port_open() { - lsof -iTCP:"$1" -sTCP:LISTEN -t >/dev/null 2>&1 -} +is_port_open() { + ss -tlnH "sport = :$1" 2>/dev/null | grep -q . || \ + (echo >/dev/tcp/127.0.0.1/"$1") 2>/dev/null +}Since this is a local-only script and macOS always has
lsof, this is low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execute-e2e-local.sh` around lines 34 - 36, The is_port_open function currently relies on lsof which may be missing; replace its body to use bash's /dev/tcp probe so it works on minimal Linux and macOS. In is_port_open, attempt a TCP connection like: redirecting stdout/stderr of a shell open to /dev/tcp/127.0.0.1/"$1" (e.g. ">/dev/tcp/127.0.0.1/$1" wrapped in a subshell) and return success (0) when the probe succeeds and non‑zero when it fails; keep the function name is_port_open and preserve the same call sites/return semantics. Ensure the probe redirects errors to /dev/null so the script output stays clean.client/e2e/application-workflow.spec.ts (1)
95-97: ReplacewaitForTimeoutwith a meaningful assertion.
page.waitForTimeout(2_000)is an arbitrary sleep — Playwright's own docs flag it as a source of flakiness and slowness. The success text locator (successText) is already in scope; use it, with a fallback URL check:♻️ Proposed refactor
- // Verify we're still on the application page (no crash) - await page.waitForTimeout(2_000) - await expect(page).toHaveURL(/\/submit-application/) + // Verify submission succeeded or at least didn't crash + await expect(successText.or(page.getByRole('heading', { name: 'Submit Application', exact: true }))).toBeVisible({ + timeout: 15_000, + }) + await expect(page).toHaveURL(/\/submit-application/)This waits for an actual DOM signal instead of sleeping, keeps the timeout deterministic, and adds a stronger success signal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/application-workflow.spec.ts` around lines 95 - 97, Replace the arbitrary sleep: remove page.waitForTimeout(2_000) and instead wait on the actual success DOM signal by asserting the existing successText locator is visible (use expect(successText).toBeVisible with an explicit timeout), and keep the existing URL assertion (expect(page).toHaveURL(/\/submit-application/)) as a fallback; update the test around the successText and page URL checks so the test waits deterministically on the DOM rather than sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/e2e/applications.spec.ts`:
- Around line 41-48: The test "review page is accessible" only asserts the URL
which can pass for error pages; update the test (inside the test block using
navigateTo and authStatePath('supervisor')) to also assert a page landmark or
heading is rendered—e.g., use page.getByRole('heading', { name: /Applications/i
}) or a main/region landmark locator and assert it is visible or has expected
text—so the test verifies the actual applications page content rather than just
the URL.
---
Duplicate comments:
In `@client/e2e/application-workflow.spec.ts`:
- Around line 67-71: The test uses an unscoped selector
page.getByRole('checkbox') (stored in privacyCheckbox) which may target the
wrong checkbox; update the selector to scope by the privacy notice label or
surrounding container so it reliably targets the privacy checkbox (e.g., use
getByRole('checkbox', { name: '...' }) or page.locator with a text
label/aria-label) and keep the subsequent isChecked() and check() calls on the
same privacyCheckbox variable (adjust the selector where privacyCheckbox is
defined).
In `@client/e2e/applications.spec.ts`:
- Around line 29-38: The reviewer confirms the advisor assertion in the test
'review page loads with application sidebar' (inside test.describe) was improved
and no code change is needed; remove the leftover duplicate review meta tags
([duplicate_comment]) from the PR description/comments and mark/approve the
change, leaving the test that uses authStatePath('advisor'), navigateTo(page,
'/applications') and the expect on page.getByPlaceholder unchanged.
In `@client/e2e/auth.setup.ts`:
- Around line 29-38: The previous unguarded JSON.parse call has been addressed
inside the page.waitForFunction block; keep the current implementation in
page.waitForFunction that retrieves
localStorage.getItem('authentication_tokens'), wraps JSON.parse(tokens) in a
try/catch, and returns !!parsed.access_token && !!parsed.refresh_token so the
e2e setup safely waits for valid tokens — no further change required.
In `@client/e2e/helpers.ts`:
- Around line 60-91: The helper searchAndSelectMultiSelect currently ignores the
_searchText parameter so server-side filtering never runs; either (A) implement
typing by renaming _searchText to searchText and calling
textbox.fill(searchText) or textbox.type(searchText) (and waiting for results)
before locating options — also repeat the fill/type in the retry branch before
re-querying retryOption — or (B) if you intend to use pre-populated dropdowns,
remove the "server-side search" docstring and rename the parameter to indicate
no typing (e.g., remove the underscore and adjust callers) so the function
behavior matches its documentation; update searchAndSelectMultiSelect
accordingly.
In `@client/e2e/interviews.spec.ts`:
- Around line 17-18: The UUID comment and the student storageState fix are
already applied; no code changes required — keep the navigateTo call to
'/interviews/00000000-0000-4000-e600-000000000001' in interviews.spec.ts and
leave the student storageState configuration as updated, then mark this change
approved (no further edits to navigateTo or storageState needed).
In `@client/e2e/presentations.spec.ts`:
- Around line 5-9: Test title was updated to match the page heading so no code
change is required; keep the test named "presentations page shows heading" and
its assertion using page.getByRole('heading', { name: 'Presentations', exact:
true }) with navigateTo(page, '/presentations') and the 15_000ms timeout as-is,
and mark the PR approved.
In `@client/e2e/research-groups.spec.ts`:
- Around line 34-42: The previous vacuous assertion was replaced with a real
check in the "supervisor can access their research group settings" test: ensure
the test uses navigateTo(...) to open the UUID path and assert visibility by
using page.getByText('Applied Software
Engineering').first().or(page.getByText(/unauthorized/i)).toBeVisible({ timeout:
15_000 }); remove any leftover placeholder/vacuous assertions or review
artefacts (e.g., duplicate_comment/approve_code_changes markers) so the test
only contains the meaningful .or(...) visibility assertion.
In `@execute-e2e-local.sh`:
- Around line 119-134: Summary: No code change required—ensure PID capture and
cleanup use the same keys. Instruction: Keep the current use of exec inside the
subshells for gradlew and webpack serve, but verify that save_pid is called
immediately after starting each backgrounded subshell (save_pid "server" $! and
save_pid "client" $!) so $! contains the actual process PID, and ensure your
cleanup function kill_pid references the same identifiers ("server" and
"client") used in save_pid to properly terminate those processes.
---
Nitpick comments:
In `@client/e2e/application-workflow.spec.ts`:
- Around line 95-97: Replace the arbitrary sleep: remove
page.waitForTimeout(2_000) and instead wait on the actual success DOM signal by
asserting the existing successText locator is visible (use
expect(successText).toBeVisible with an explicit timeout), and keep the existing
URL assertion (expect(page).toHaveURL(/\/submit-application/)) as a fallback;
update the test around the successText and page URL checks so the test waits
deterministically on the DOM rather than sleeping.
In `@client/e2e/presentations.spec.ts`:
- Around line 25-31: The test 'presentation detail page is accessible without
login' uses a hardcoded seed UUID in the navigateTo call (navigateTo(page,
'/presentations/00000000-0000-4000-e300-000000000002')) but lacks the
explanatory seed-coupling comment present in interviews.spec.ts; add a short
comment above that line stating this UUID is a seeded public presentation
(thesis 3, final, public visibility) and that the test relies on the seeded DB
so future maintainers know the dependency.
In `@client/e2e/research-groups.spec.ts`:
- Around line 20-28: The test "search filters research groups" currently only
asserts that the matching group remains visible; add a negative assertion to
verify non-matching groups are hidden after filling the search. After calling
navigateTo and filling the placeholder via page.getByPlaceholder(/search
research groups/i).fill('Applied Software'), add an expectation using
page.getByText(...) for a known non-matching group (or use a selector that
matches all groups) and assert .not.toBeVisible() or .toHaveCount(0). Keep the
existing assertions (heading visible and matching group visible) and place the
negative assertion right after the positive check so the test validates both
presence and absence.
In `@execute-e2e-local.sh`:
- Around line 34-36: The is_port_open function currently relies on lsof which
may be missing; replace its body to use bash's /dev/tcp probe so it works on
minimal Linux and macOS. In is_port_open, attempt a TCP connection like:
redirecting stdout/stderr of a shell open to /dev/tcp/127.0.0.1/"$1" (e.g.
">/dev/tcp/127.0.0.1/$1" wrapped in a subshell) and return success (0) when the
probe succeeds and non‑zero when it fails; keep the function name is_port_open
and preserve the same call sites/return semantics. Ensure the probe redirects
errors to /dev/null so the script output stays clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
e2e_tests.yml) that spins up PostgreSQL + Keycloak, seeds test data, and runs the full test suiteexecute-e2e-local.sh) for one-command test executiondocs/DEVELOPMENT.mdwith execution instructions, test structure, role coverage, and a coverage analysis tableWhat the tests cover
Key implementation details
checkLoginIframecheck in keycloak-js--ui/--headed/--stopflags, fully idempotentTest plan
npm run e2e— 54 including setup)execute-e2e-local.shworks on a clean checkoutSummary by CodeRabbit
Tests
Chores
Documentation