E2e test fw lite#1866
Open
hahn-kev wants to merge 24 commits into
Open
Conversation
github-actions
Bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds e2e testing infrastructure for FwLiteWeb against a lexbox cluster: an FwLiteLauncher process manager, e2e fixtures/helpers/tests, a new CI job with Playwright caching, backend health-check/delete endpoints, an auth returnUrl fix, and refactors existing UI tests to use new ProjectPage/DemoProjectPage page objects. ChangesBackend endpoints
E2E test infrastructure and CI pipeline
UI test refactor and viewer cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Contributor
UI unit Tests 1 files 62 suites 31s ⏱️ Results for commit ca591be. ♻️ This comment has been updated with latest results. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
C# Unit Tests165 tests 165 ✅ 21s ⏱️ Results for commit ca591be. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Merged
myieye
force-pushed
the
e2e-test-fw-lite
branch
from
3a3f357 to
f79a76d
Compare
github-actions
Bot
added
the
📦 Lexbox
Spins up a real `FwLiteWeb` binary against a kind-cluster lexbox in CI and
drives it from a Playwright-controlled Chromium. Two scenarios so far: a
smoke test (login + see server projects) and a full project-download test
(triggers an initial CRDT sync via the lexbox API, then downloads sena-3
through the FwLite UI). Both run end-to-end against a fresh kind cluster.
Test infrastructure
- New `frontend/viewer/tests/e2e/` directory with playwright.config.ts,
fixtures, helpers, and the integration test file.
- Existing snapshot tests moved to `tests/snapshots/`; package.json scripts
split into `test:snapshots` and `test:e2e`.
- New vitest `integration` project for `tests/integration/fw-lite-launcher.test.ts`
(node env, hard-fails when the binary is missing). `pnpm test` excludes
it via `--project=!integration` so unrelated jobs don't need the binary.
- Vitest `browser` project restored (was dropped accidentally during
earlier cleanup) so existing `*.browser.test.ts` files keep running.
Workflow (`.github/workflows/fw-lite.yaml`)
- New `e2e-test` job depending only on `frontend` (fast-lane — parallel
with `build-and-test`). Does its own `dotnet publish -r linux-x64` so
it doesn't wait for `publish-linux`.
- Spins up lexbox via `setup-k8s`, then port-forwards ingress-nginx :443
to localhost:6580 (MSAL.NET refuses non-HTTPS authorities; the snake-oil
cert is trusted via `--environment Development` and playwright's
`ignoreHTTPSErrors: true`).
- On failure, uploads playwright traces/screenshots/videos AND a dump of
k8s pod logs (`kubectl describe`/`logs` for lexbox/ui/hg/db/fw-headless/
ingress-nginx) — pattern lifted from `integration-test-gha.yaml`.
- Caches `~/.cache/ms-playwright` keyed on pnpm-lock hash across the three
jobs that run `playwright install --with-deps` (frontend,
frontend-component-unit-tests, e2e-test).
- Adds `permissions:` blocks to satisfy least-privilege guidelines.
Backend (FwLite)
- `FwLiteWeb`: `/health` endpoint (used by the launcher's readiness probe),
`DELETE /api/crdt/{code}` for per-test cleanup, stdin-triggered shutdown
(Windows can't SIGTERM child processes; launcher writes "shutdown\n").
- `AuthRoutes`: redirect-after-login fallback to `/` when the referer is
the auth endpoint itself.
- `HomeView.svelte` / `Server.svelte`: stable anchor IDs (`#local-projects`,
`id={server?.id}`) for Playwright selectors.
Deployment
- `deployment/gha/lexbox.patch.yaml`: trust-all `KnownNetworks` override so
lexbox honors `X-Forwarded-Proto: https` from the kind ingress controller
(without this, lexbox emits HTTP URLs in `/.well-known/openid-configuration`
even though the request came in over HTTPS, breaking the OAuth redirect).
Iteration history is preserved in branch `e2e-test-fw-lite-backup-pre-squash`.
- Poll for entry rows in ProjectPage.waitFor: the table shell renders before all entries hydrate, so a one-shot count was racing. - afterEach: navigate back to home before logout. The download test ends on the project page, where serverSection isn't visible. - fw-lite workflow: cancel-in-progress concurrency group so iterating on a PR doesn't stack up Windows + e2e lanes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The shadcnMode-removal commit (3c30769) dropped the `<script module>` block, which also held `import {onMount} from 'svelte'`. The regular script still calls `onMount(() => onloaded(true))`, so without the import the page crashed on mount, `onloaded` never fired, ProjectLoader never resolved, and all 21 UI tests timed out waiting for the Filter textbox. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract serverUrl(server) helper; drop duplicated URL building - Inline single-use logoutFromServer wrapper - Extract launchConfig() helper in launcher tests - Name launcher constants (DEFAULT_LAUNCH_TIMEOUT_MS, READY_STDOUT_MARKERS) - Fix latent hang in FwLiteLauncher.spawnProcess: clean early exits (code=0 before the listening marker) now reject instead of hanging - Tighten JSDoc, keeping the substantive why Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # frontend/viewer/.gitignore # frontend/viewer/tests/entries-list.test.ts # frontend/viewer/tests/pages/entries-list.component.ts # frontend/viewer/tests/test-utils.ts
- fw-lite.yaml e2e-test job: fix dotnet-version 9.x→10.x (matches net10 TFM the publish step builds), SHA-pin the floating @v4 actions, add a job timeout - ProjectRoutes DELETE /crdt/{code}: return 404 instead of an unhandled 500 when the project doesn't exist - AuthRoutes: Allman braces + ordinal StartsWith - drop stale enableShadcn global decl (assignment removed with shadcnMode) - fw-lite-launcher: clear isHealthy/process on exit so isRunning() reflects a post-launch crash - note why home.page.ts reuses the parent app's LoginPage Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- e2e-test job: bump pnpm/action-setup v4 → v5 to match the other jobs - root AGENTS.md: update renamed task playwright-test-standalone → test:ui-standalone Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Remove commented-out reporter block in tests/ui/playwright.config.ts - Trim two narrating test comments to their non-obvious point - Restore the workflow concurrency group (lost in an earlier refactor), guarded so it only cancels superseded PR runs, never release builds Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keys must be the real server project codes (elawa-dev-flex, empty-dev-flex per SeedingData.cs), not the shortened forms — otherwise ensureProjectCrdtReady throws "Unknown project code" for any test targeting those projects. Co-Authored-By: Claude Opus 4.8 <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: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deployment/gha/lexbox.patch.yaml`:
- Around line 14-19: The forwarded-header trust setting is too broad because
ForwardedHeadersOptions__KnownNetworks__0 currently allows every IPv4 source.
Update the lexbox deployment env/config to use the actual kind/ingress pod or
service CIDR, or specific ingress proxy addresses, so only the intended proxy
network can supply X-Forwarded-* values. Keep the change anchored around the
ForwardedHeadersOptions__KnownNetworks__0 entry in the lexbox patch and ensure
it matches the real cluster network instead of 0.0.0.0/0.
In `@frontend/viewer/AGENTS.md`:
- Around line 42-52: Remove the unfiltered UI test workflow from the viewer docs
so only filtered Playwright runs are recommended. In AGENTS.md, update the UI
tests section to keep the `task test:ui-standalone -- <test-name-filter>`
guidance and delete the `task test:ui-standalone` “All UI tests” entry, so the
documented workflow matches the viewer test rule.
In `@frontend/viewer/Taskfile.yml`:
- Around line 63-70: Update the launcher build task so it matches the default
path expected by the viewer tests on Unix. The issue is in the
`test:build-launcher` task output under `Taskfile.yml`: it currently publishes
`FwLiteWeb` into `./dist/fw-lite-server`, but `test:launcher` still assumes the
binary lives at `./dist/fw-lite-server/FwLiteWeb.exe`. Fix this by either
changing `test:build-launcher` to place the built artifact at the exact path
expected by the launcher config, or by updating the launcher test path contract
consistently in the related `test:launcher` / `FW_LITE_BINARY_PATH` setup so the
task works without manual env overrides on Linux/macOS.
In `@frontend/viewer/tests/e2e/config.ts`:
- Around line 10-12: The env-derived server config in the config export should
be validated before use instead of trusting casts and partial parsing. Update
the config building logic around the hostname/protocol/port values to explicitly
validate TEST_SERVER_PROTOCOL and TEST_SERVER_PORT, rejecting anything outside
the allowed http/https values or non-integer/out-of-range ports with a clear
error. Use the existing config constants in config.ts so invalid environment
values fail fast during E2E startup rather than causing later failures.
In `@frontend/viewer/tests/e2e/helpers/fixtures.ts`:
- Around line 9-29: The fwLite fixture setup can leave the FwLiteLauncher
process running if launch(), page.goto(), or
page.waitForLoadState('networkidle') throws before use(launcher) is reached.
Move the teardown into a try/finally around the entire setup-and-use flow in the
fwLite fixture so launcher.shutdown() always runs, even when initialization
fails; use the existing fwLite fixture and FwLiteLauncher symbols to keep the
fix localized.
In `@frontend/viewer/tests/e2e/helpers/fw-lite-launcher.ts`:
- Around line 29-39: The launch flow in FWLiteLauncher leaves an orphaned child
process when spawnProcess() succeeds but waitForHealthy() times out. Update
launch() to catch startup/health-check failures, terminate the spawned process,
and clear this.process before rethrowing, using the existing launch(),
spawnProcess(), waitForHealthy(), and shutdown()/process cleanup logic so later
tests don’t inherit a stale FW Lite instance.
- Around line 41-58: The shutdown flow in fw-lite-launcher’s shutdown method is
using proc.killed to decide whether to escalate on timeout, but that flag
becomes true immediately after SIGTERM is sent rather than when the child
actually exits. Update the shutdown logic to track a separate exit state from
the proc.once('exit') handler, and use that state in the timedOut branch so a
still-running process is force-killed with SIGKILL after SHUTDOWN_TIMEOUT_MS.
In `@frontend/viewer/tests/e2e/helpers/project-operations.ts`:
- Around line 4-7: The cleanup in deleteProject is deriving its target from
page.url(), which can point at the wrong host during the OAuth flow. Update
deleteProject to use the configured viewer/server origin for the DELETE request
instead of computing it from the current Page URL, while keeping the best-effort
cleanup behavior. Use the existing deleteProject helper as the place to switch
to the shared server origin source so the request always hits the correct
backend.
In `@frontend/viewer/tests/e2e/smoke.test.ts`:
- Around line 9-10: The smoke test in the post-login assertion is checking the
server project count too early, before the remote list has finished loading.
Update the test around homePage.ensureLoggedIn and
homePage.serverProjects(lexboxServer) to poll until the count is non-zero
instead of reading it once, using the existing homePage/serverProjects helpers
so the assertion waits for data to arrive.
In `@frontend/viewer/tests/pages/home.page.ts`:
- Around line 83-85: The download completion check in the home page test is too
strict because it waits for the loading indicator to appear, which can race with
fast imports. Update the flow in home.page.ts around the progressIndicator wait
logic so it does not require `.i-mdi-loading` to render, and instead rely on the
existing localProjectLink assertion as the signal that the import finished
successfully.
In `@frontend/viewer/tests/pages/project.page.ts`:
- Around line 18-27: The shared ProjectPage.waitFor() method currently
hard-codes an entries count threshold with entriesList.entryRows.count(), which
bakes a dataset assumption into the page object. Remove the “greater than 5”
requirement and replace it with a load-complete check that does not depend on
how many rows are visible, using the existing waitFor() flow and
entriesList.entryRows locator only to confirm hydration if needed.
In `@frontend/viewer/tests/ui/entries-list.test.ts`:
- Around line 70-71: The test in entries-list.test.ts still uses a fixed sleep
via waitForTimeout, which should be replaced with a deterministic state-based
wait. Update the affected assertion flow around projectPage.page and
projectPage.entriesList to wait for the UI state directly, using the existing
helpers in this file such as waitForSkeletonsToResolve(), expect.poll, or toPass
so the check only proceeds once skeletons are actually ready.
In `@frontend/viewer/tests/ui/playwright.config.ts`:
- Around line 6-7: The AUTO_START_SERVER handling in the Playwright config is
parsing environment input with Boolean(), which incorrectly treats values like
"false" and "0" as enabled. Update the serverPort setup to read
AUTO_START_SERVER as a real boolean by checking for the explicit string value
"true", and keep the existing vitePort/dotnetPort selection logic tied to that
parsed flag in the config entry that defines autoStartServer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9befb08c-a773-41cb-8526-411c9b44b851
📒 Files selected for processing (40)
.github/workflows/fw-lite.yamlAGENTS.mdbackend/FwLite/FwLiteWeb/FwLiteWebServer.csbackend/FwLite/FwLiteWeb/Routes/AuthRoutes.csbackend/FwLite/FwLiteWeb/Routes/ProjectRoutes.csdeployment/gha/lexbox.patch.yamlfrontend/viewer/.gitignorefrontend/viewer/AGENTS.mdfrontend/viewer/Taskfile.ymlfrontend/viewer/package.jsonfrontend/viewer/src/ShadcnProjectView.sveltefrontend/viewer/src/app.d.tsfrontend/viewer/src/home/Server.sveltefrontend/viewer/tests/browse-page.tsfrontend/viewer/tests/e2e/config.tsfrontend/viewer/tests/e2e/helpers/fixtures.tsfrontend/viewer/tests/e2e/helpers/fw-lite-launcher.tsfrontend/viewer/tests/e2e/helpers/project-operations.tsfrontend/viewer/tests/e2e/playwright.config.tsfrontend/viewer/tests/e2e/project-download.test.tsfrontend/viewer/tests/e2e/smoke.test.tsfrontend/viewer/tests/entries-list.test.tsfrontend/viewer/tests/launcher/fw-lite-launcher.test.tsfrontend/viewer/tests/pages/entries-list.component.tsfrontend/viewer/tests/pages/entry-view.component.tsfrontend/viewer/tests/pages/home.page.tsfrontend/viewer/tests/pages/project.page.tsfrontend/viewer/tests/test-utils.tsfrontend/viewer/tests/ui/back-navigation-persistence.test.tsfrontend/viewer/tests/ui/demo-project.page.tsfrontend/viewer/tests/ui/entries-list.test.tsfrontend/viewer/tests/ui/entry-api-helper.tsfrontend/viewer/tests/ui/entry-edit-persists.test.tsfrontend/viewer/tests/ui/playwright.config.tsfrontend/viewer/tests/ui/snapshots/project-view-snapshots.test.tsfrontend/viewer/tests/ui/snapshots/snapshot.tsfrontend/viewer/tests/ui/tab-focus-preservation.test.tsfrontend/viewer/tests/ui/test.d.tsfrontend/viewer/tsconfig.node.jsonfrontend/viewer/vitest.config.ts
💤 Files with no reviewable changes (4)
- frontend/viewer/tests/test-utils.ts
- frontend/viewer/tests/browse-page.ts
- frontend/viewer/src/app.d.ts
- frontend/viewer/tests/entries-list.test.ts
Collaborator
Author
|
@myieye man there's a lot here. I've taken a look at it and it looks good. But I can't say I read every line. |
- Escalate to SIGKILL on shutdown timeout via a real exit flag (proc.killed reflects only that a signal was sent, so it never escalated) - Tear down the launcher when launch() fails healthy, and when fixture setup throws before use() — both previously orphaned the FwLiteWeb process - Target the FwLite host explicitly for CRDT cleanup instead of page origin - Poll for server projects / the downloaded local link instead of racing a one-shot count and the transient loading spinner - Platform-aware default binary path (FwLiteWeb vs .exe) for local Unix runs - Parse AUTO_START_SERVER as === 'true' (Boolean(str) was always truthy) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
approved these changes
Jul 3, 2026
myieye
left a comment
myieye
left a comment
Collaborator
There was a problem hiding this comment.
I'm now happy with this PR, assuming CI is now green and the argos screenshots are working.
Potentially we want to migrate some other playwright tests to run as E2E tests, but I think that can be a follow up.
Under the shared default name the e2e build compared against the ui suite's baseline, marking its 8 screenshots as removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
3 participants
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.
Resolves: #1511
Kevin: I started working on this with Kiro, but then ditched it as it was going in the wrong direction and creating a bunch of fluff