fix(test): make browser-project tests contribute to coverage measurement #495

marcel · 2026-05-09T18:05:37+02:00

marcel commented

2026-05-09 18:05:37 +02:00

Closes #425

Summary

Installs @vitest/coverage-istanbul@4.1.0 — the only provider that works inside Chromium's sandbox (v8 is silently a no-op in browser mode)
Adds vitest.client-coverage.config.ts, a standalone Vitest config that runs the browser project with Istanbul coverage writing to coverage/client/; a standalone file is required because Vitest 4 ignores per-project coverage overrides inside test.projects
Updates the root vite.config.ts coverage block to write server output to coverage/server/ (was coverage/)
Updates test:coverage to run both projects sequentially (&&), eliminating the ENOTEMPTY race on coverage/.tmp
Adds a hard coverage gate to the CI unit-tests job — threshold failure exits non-zero; both lcov reports are uploaded as a coverage-reports artifact

Coverage numbers at merge

Project	Provider	Branch coverage	Threshold
server	v8	~94%	80% ✅
client	Istanbul	~53%	80% ❌

The client project starts below 80% (all Svelte components included, as decided in Theme 1). This will fail CI until component test coverage is raised — consistent with Theme 3 (hard gate from day one).

Test plan

npm test — 190 files, 1850 tests, all green
npm run test:coverage — produces coverage/server/lcov.info (v8) and coverage/client/lcov.info (Istanbul)
coverage/client/lcov.info references .svelte component files (not server utilities)
Server threshold ≥ 80% branches → passes
Client threshold < 80% branches → exits non-zero (expected, hard gate)

🤖 Generated with Claude Code

Closes #425 ## Summary - Installs `@vitest/coverage-istanbul@4.1.0` — the only provider that works inside Chromium's sandbox (v8 is silently a no-op in browser mode) - Adds `vitest.client-coverage.config.ts`, a standalone Vitest config that runs the browser project with Istanbul coverage writing to `coverage/client/`; a standalone file is required because Vitest 4 ignores per-project `coverage` overrides inside `test.projects` - Updates the root `vite.config.ts` coverage block to write server output to `coverage/server/` (was `coverage/`) - Updates `test:coverage` to run both projects sequentially (`&&`), eliminating the `ENOTEMPTY` race on `coverage/.tmp` - Adds a hard coverage gate to the CI `unit-tests` job — threshold failure exits non-zero; both lcov reports are uploaded as a `coverage-reports` artifact ## Coverage numbers at merge | Project | Provider | Branch coverage | Threshold | |---------|----------|-----------------|-----------| | server | v8 | ~94% | 80% ✅ | | client | Istanbul | ~53% | 80% ❌ | The client project starts below 80% (all Svelte components included, as decided in Theme 1). This will fail CI until component test coverage is raised — consistent with Theme 3 (hard gate from day one). ## Test plan - [ ] `npm test` — 190 files, 1850 tests, all green - [ ] `npm run test:coverage` — produces `coverage/server/lcov.info` (v8) and `coverage/client/lcov.info` (Istanbul) - [ ] `coverage/client/lcov.info` references `.svelte` component files (not server utilities) - [ ] Server threshold ≥ 80% branches → passes - [ ] Client threshold < 80% branches → exits non-zero (expected, hard gate) 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel added 4 commits 2026-05-09 18:05:38 +02:00

build(deps): add @vitest/coverage-istanbul for browser-project coverage 1a28e3114d

Istanbul instruments code at transpile time and works inside Chromium's
sandbox; v8 coverage is silently a no-op in browser mode.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(test): add Istanbul browser coverage via standalone client config bb374bf2cd

Vitest 4 silently ignores per-project coverage overrides in test.projects,
so a standalone vitest.client-coverage.config.ts provides the root-level
Istanbul coverage block that Vitest actually honours.

Root vite.config.ts retains the v8 coverage block (reportsDirectory:
coverage/server) for the server project. The client config writes to
coverage/client and instruments all .svelte and .svelte.ts files.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(test): update test:coverage to run both server and client projects 16f69fff33

Sequential && prevents the ENOTEMPTY race on coverage/.tmp. Server
uses v8 via --project=server; client uses the standalone Istanbul config.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ci: add combined coverage gate to unit-tests job

CI / Unit & Component Tests (push) Failing after 5m54s

Details

CI / Backend Unit Tests (push) Failing after 3m20s

Details

CI / Unit & Component Tests (pull_request) Failing after 5m48s

Details

CI / OCR Service Tests (push) Successful in 38s

Details

CI / OCR Service Tests (pull_request) Successful in 33s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m21s

Details

eccecf35e3

Runs test:coverage (server v8 + client Istanbul) after tests, hard-gates
on both 80% branch thresholds, and uploads coverage/ as an artifact.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this pull request

2026-05-09 18:05:58 +02:00

fix(test): make browser-project tests contribute to coverage measurement #425

marcel commented

2026-05-09 18:46:22 +02:00

👷 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

CI workflow step ordering and action versions
Artifact upload behavior
Sequential vs parallel coverage execution

What's done well

actions/upload-artifact@v4 — correct, current version. No deprecated v3 sneaking in.

if: always() on the upload step is exactly right. Coverage artifacts get uploaded even when the coverage gate fails, which means I can inspect reports on a failing build instead of flying blind. Good defensive move.

Sequential && in test:coverage — the PR description explains the ENOTEMPTY race on coverage/.tmp that this solves. That's a real-world CI race condition and the fix is the right one. Parallelism would save maybe 30 seconds but cause intermittent failures.

Two separate output directories (coverage/server/, coverage/client/) — clean separation. One artifact archive uploads both, which keeps the artifact count low and the download simple.

Minor observation

The coverage step runs on every CI push. The client run starts Playwright (Chromium headless), which has a non-trivial startup time. If CI minutes become a concern later, consider caching the Playwright browser binary more aggressively (it may already be cached via the node_modules cache key). Not a blocker — just something to monitor.

One heads-up on the failing gate

The PR ships knowing the client threshold fails at ~53%. CI will exit non-zero on test:coverage until component coverage reaches 80%. The Run coverage (server + client) step will turn red on every PR to main until that's fixed. That's a valid "hard gate from day one" strategy, but the team (in this case, just you) needs to be aware that all merges to main will have this step failing in the log until coverage is addressed. Not a blocker for this PR — it's an intentional decision — but worth tracking as a known issue.

## 👷 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked - CI workflow step ordering and action versions - Artifact upload behavior - Sequential vs parallel coverage execution ### What's done well **`actions/upload-artifact@v4`** — correct, current version. No deprecated v3 sneaking in. **`if: always()`** on the upload step is exactly right. Coverage artifacts get uploaded even when the coverage gate fails, which means I can inspect reports on a failing build instead of flying blind. Good defensive move. **Sequential `&&` in `test:coverage`** — the PR description explains the `ENOTEMPTY` race on `coverage/.tmp` that this solves. That's a real-world CI race condition and the fix is the right one. Parallelism would save maybe 30 seconds but cause intermittent failures. **Two separate output directories** (`coverage/server/`, `coverage/client/`) — clean separation. One artifact archive uploads both, which keeps the artifact count low and the download simple. ### Minor observation The coverage step runs on every CI push. The client run starts Playwright (Chromium headless), which has a non-trivial startup time. If CI minutes become a concern later, consider caching the Playwright browser binary more aggressively (it may already be cached via the node_modules cache key). Not a blocker — just something to monitor. ### One heads-up on the failing gate The PR ships knowing the client threshold fails at ~53%. CI will exit non-zero on `test:coverage` until component coverage reaches 80%. The `Run coverage (server + client)` step will turn red on every PR to main until that's fixed. That's a valid "hard gate from day one" strategy, but the team (in this case, just you) needs to be aware that all merges to main will have this step failing in the log until coverage is addressed. Not a blocker for this PR — it's an intentional decision — but worth tracking as a known issue.

marcel commented

2026-05-09 18:46:37 +02:00

👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

What I checked

vitest.client-coverage.config.ts — structure, correctness, duplication
test:coverage script composition
Coverage thresholds completeness
Code comments and naming

Blocker

Client coverage config only gates branches: 80 — no lines, functions, or statements thresholds.

The current vitest.client-coverage.config.ts:

thresholds: {
  branches: 80
}

This means a Svelte component could have 80% branch coverage but 10% line coverage (e.g., all branches in a single if-block are tested, but most render paths are never hit). Istanbul's branch metric alone does not guarantee meaningful test coverage. The server config likely inherits the same gap via the root vite.config.ts.

Suggest extending to:

thresholds: {
  lines: 80,
  functions: 80,
  branches: 80,
  statements: 80
}

Not asking for a stricter gate — same 80% across all dimensions — but the current setup will pass coverage with misleadingly sparse tests.

Suggestions (non-blocking)

Config duplication: vitest.client-coverage.config.ts re-declares the full Vite plugin stack (Tailwind, SvelteKit, devtoolsJson, Paraglide) that's already in vite.config.ts. The comment explains why (Vitest 4 ignores per-project coverage overrides) — that's exactly the right kind of comment. But the duplication is real tech debt: if a plugin is added or reconfigured in vite.config.ts, the coverage config won't pick it up automatically.

Minimal fix for now: add a note to the file header listing which plugins are mirrored from vite.config.ts so future-you knows what to keep in sync:

// Plugins mirrored from vite.config.ts: tailwindcss, sveltekit, devtoolsJson, paraglideVitePlugin
// Update here whenever vite.config.ts plugins change.

requireAssertions: true — good catch, this is correct hygiene for a browser test project where assertions could easily be swallowed by async mis-wiring.

exclude: ['src/lib/server/**'] in the client include pattern — sensible. Server-only code has no business being in the browser coverage report.

The test:coverage sequential composition (server && client) is clean and the problem it solves (ENOTEMPTY race) is documented. KISS wins here.

## 👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### What I checked - `vitest.client-coverage.config.ts` — structure, correctness, duplication - `test:coverage` script composition - Coverage thresholds completeness - Code comments and naming --- ### Blocker **Client coverage config only gates `branches: 80` — no `lines`, `functions`, or `statements` thresholds.** The current `vitest.client-coverage.config.ts`: ```ts thresholds: { branches: 80 } ``` This means a Svelte component could have 80% branch coverage but 10% line coverage (e.g., all branches in a single if-block are tested, but most render paths are never hit). Istanbul's branch metric alone does not guarantee meaningful test coverage. The server config likely inherits the same gap via the root `vite.config.ts`. Suggest extending to: ```ts thresholds: { lines: 80, functions: 80, branches: 80, statements: 80 } ``` Not asking for a stricter gate — same 80% across all dimensions — but the current setup will pass coverage with misleadingly sparse tests. --- ### Suggestions (non-blocking) **Config duplication**: `vitest.client-coverage.config.ts` re-declares the full Vite plugin stack (Tailwind, SvelteKit, devtoolsJson, Paraglide) that's already in `vite.config.ts`. The comment explains *why* (Vitest 4 ignores per-project coverage overrides) — that's exactly the right kind of comment. But the duplication is real tech debt: if a plugin is added or reconfigured in `vite.config.ts`, the coverage config won't pick it up automatically. Minimal fix for now: add a note to the file header listing which plugins are mirrored from `vite.config.ts` so future-you knows what to keep in sync: ```ts // Plugins mirrored from vite.config.ts: tailwindcss, sveltekit, devtoolsJson, paraglideVitePlugin // Update here whenever vite.config.ts plugins change. ``` **`requireAssertions: true`** — good catch, this is correct hygiene for a browser test project where assertions could easily be swallowed by async mis-wiring. **`exclude: ['src/lib/server/**']`** in the client include pattern — sensible. Server-only code has no business being in the browser coverage report. The `test:coverage` sequential composition (`server && client`) is clean and the problem it solves (ENOTEMPTY race) is documented. KISS wins here.

marcel commented

2026-05-09 18:46:51 +02:00

🏛️ Markus Keller (@mkeller) — Senior Application Architect

Verdict: ✅ Approved

What I checked

Architectural justification for two-provider coverage approach
Documentation update requirements (per the doc-update matrix)
Config duplication as structural tech debt
CI gate strategy

Documentation update matrix

This PR changes:

Frontend tooling configuration (Vitest, CI)
No new routes, controllers, services, domain modules, migrations, Docker services, or external systems

Result: no doc updates required. Nothing in the CLAUDE.md doc-update matrix is triggered.

Architectural assessment

Two-provider approach is justified and well-reasoned. The PR explanation — v8 is silently a no-op inside Chromium's sandbox, Istanbul instruments source at the AST level and is sandbox-agnostic — is correct. This is not a workaround for a bad design choice; it's the correct tool for the constraints.

The standalone config file is a well-constrained workaround. Vitest 4's limitation (ignoring per-project coverage overrides inside test.projects) forces the duplication. The comment in vitest.client-coverage.config.ts documents exactly why the file exists. If Vitest fixes this in a future release, the migration path is clear: remove the standalone file and move coverage config back into the project definition. That's a good structure — the complexity is isolated and the exit path is known.

Hard gate from day one is architecturally sound. Shipping the gate below threshold is the correct approach versus either (a) setting a lower threshold that doesn't reflect the goal, or (b) delaying the gate until coverage is higher. Starting with a failing gate creates immediate, visible pressure to raise coverage rather than deferring it indefinitely. The gate is transparent — the PR description documents exactly where coverage stands and what will fix it.

One concern worth flagging: the coverage/ root directory path changed from coverage/ to coverage/server/. If any downstream tooling (e.g., a future SonarQube integration, a code coverage badge, or a PR bot comment) was pointed at coverage/lcov.info, it will break silently. This is unlikely to matter now in a solo project, but worth noting as a future integration gotcha.

No ADR needed

The Istanbul vs v8 choice for browser testing is a tooling constraint, not an architectural decision with lasting system-design consequences. An ADR would be overkill here. The PR description itself serves as the decision record.

## 🏛️ Markus Keller (@mkeller) — Senior Application Architect **Verdict: ✅ Approved** ### What I checked - Architectural justification for two-provider coverage approach - Documentation update requirements (per the doc-update matrix) - Config duplication as structural tech debt - CI gate strategy --- ### Documentation update matrix This PR changes: - Frontend tooling configuration (Vitest, CI) - No new routes, controllers, services, domain modules, migrations, Docker services, or external systems **Result: no doc updates required.** Nothing in the CLAUDE.md doc-update matrix is triggered. --- ### Architectural assessment **Two-provider approach is justified and well-reasoned.** The PR explanation — v8 is silently a no-op inside Chromium's sandbox, Istanbul instruments source at the AST level and is sandbox-agnostic — is correct. This is not a workaround for a bad design choice; it's the correct tool for the constraints. **The standalone config file is a well-constrained workaround.** Vitest 4's limitation (ignoring per-project `coverage` overrides inside `test.projects`) forces the duplication. The comment in `vitest.client-coverage.config.ts` documents exactly why the file exists. If Vitest fixes this in a future release, the migration path is clear: remove the standalone file and move coverage config back into the project definition. That's a good structure — the complexity is isolated and the exit path is known. **Hard gate from day one is architecturally sound.** Shipping the gate below threshold is the correct approach versus either (a) setting a lower threshold that doesn't reflect the goal, or (b) delaying the gate until coverage is higher. Starting with a failing gate creates immediate, visible pressure to raise coverage rather than deferring it indefinitely. The gate is transparent — the PR description documents exactly where coverage stands and what will fix it. **One concern worth flagging:** the `coverage/` root directory path changed from `coverage/` to `coverage/server/`. If any downstream tooling (e.g., a future SonarQube integration, a code coverage badge, or a PR bot comment) was pointed at `coverage/lcov.info`, it will break silently. This is unlikely to matter now in a solo project, but worth noting as a future integration gotcha. --- ### No ADR needed The Istanbul vs v8 choice for browser testing is a tooling constraint, not an architectural decision with lasting system-design consequences. An ADR would be overkill here. The PR description itself serves as the decision record.

marcel commented

2026-05-09 18:47:02 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

What I checked

Supply chain risk from new Istanbul / Babel dependency tree
CI artifact security
Secrets or credentials in workflow YAML
Security implications of changed coverage paths

No security issues found

This PR is pure test infrastructure. The changed files are:

A CI workflow adding coverage steps
A new Vitest config file
package.json and package-lock.json adding @vitest/coverage-istanbul

Supply chain assessment: @vitest/coverage-istanbul@4.1.0 brings in a standard Babel toolchain (@babel/core, @babel/traverse, etc.) and Istanbul's own packages (istanbul-lib-coverage, istanbul-lib-report, istanbul-reports). These are all well-established, widely-used packages with active maintenance. The Babel dependency pinned to ^7.29.0 is current and not known-vulnerable. All new packages are devDependencies — they do not ship in the production bundle.

No hardcoded secrets in the CI workflow. The coverage step uses only TZ: Europe/Berlin as an env var — not sensitive.

if: always() on artifact upload is fine — coverage reports don't contain secrets or sensitive data. They reference source paths and coverage percentages.

Coverage output paths (coverage/server/, coverage/client/) are local ephemeral directories uploaded as CI artifacts. No security implications.

One minor observation (not a vulnerability)

The @vitest/coverage-istanbul package pulls in obug@^2.1.1 as a dependency (visible in the lock file). This is an unusual dependency name — worth verifying it's the legitimate Istanbul obfuscation helper and not a typosquat. A quick npm audit on the frontend directory would confirm no known CVEs in the new dependency tree, which I'd suggest running before merge.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### What I checked - Supply chain risk from new Istanbul / Babel dependency tree - CI artifact security - Secrets or credentials in workflow YAML - Security implications of changed coverage paths --- ### No security issues found This PR is pure test infrastructure. The changed files are: - A CI workflow adding coverage steps - A new Vitest config file - package.json and package-lock.json adding `@vitest/coverage-istanbul` **Supply chain assessment**: `@vitest/coverage-istanbul@4.1.0` brings in a standard Babel toolchain (`@babel/core`, `@babel/traverse`, etc.) and Istanbul's own packages (`istanbul-lib-coverage`, `istanbul-lib-report`, `istanbul-reports`). These are all well-established, widely-used packages with active maintenance. The Babel dependency pinned to `^7.29.0` is current and not known-vulnerable. All new packages are `devDependencies` — they do not ship in the production bundle. **No hardcoded secrets** in the CI workflow. The coverage step uses only `TZ: Europe/Berlin` as an env var — not sensitive. **`if: always()`** on artifact upload is fine — coverage reports don't contain secrets or sensitive data. They reference source paths and coverage percentages. **Coverage output paths** (`coverage/server/`, `coverage/client/`) are local ephemeral directories uploaded as CI artifacts. No security implications. --- ### One minor observation (not a vulnerability) The `@vitest/coverage-istanbul` package pulls in `obug@^2.1.1` as a dependency (visible in the lock file). This is an unusual dependency name — worth verifying it's the legitimate Istanbul obfuscation helper and not a typosquat. A quick `npm audit` on the frontend directory would confirm no known CVEs in the new dependency tree, which I'd suggest running before merge.

marcel commented

2026-05-09 18:47:18 +02:00

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

What I checked

Coverage threshold completeness and accuracy
Test file inclusion/exclusion patterns
CI gate behavior with intentionally failing threshold
Quality gate strategy

Blockers

1. Only branches: 80 threshold — coverage quality gap

vitest.client-coverage.config.ts only asserts on branch coverage:

thresholds: { branches: 80 }

This means a Svelte component with 80% branch coverage but 5% line coverage passes the gate. For a browser component test suite, lines and functions are the most meaningful coverage dimensions (you want to know: is the component's render logic actually executed?). Recommend aligning thresholds across all four dimensions at the same 80% floor:

thresholds: {
  lines: 80,
  functions: 80,
  branches: 80,
  statements: 80
}

2. CI gate fails at ~53% client coverage — every merge to main will have a red CI step

The PR explicitly documents this as "hard gate from day one." I understand and respect the intent: creating immediate pressure to raise coverage rather than deferring it. But the practical consequence is that CI shows a red step on every PR until coverage reaches 80%. This can normalize CI red states and make it harder to notice new failures.

Suggestion: Either (a) accept this and treat the coverage step as a known-failing gate with a tracked issue, or (b) start the threshold at the current baseline (~53%) and ratchet it up each sprint. Either approach is valid — I just want it explicitly decided, not accidentally tolerated.

What's done well

requireAssertions: true on the browser project config — excellent. This catches async test patterns where the expect never runs (the test body completes before the assertion fires). This is a common pitfall in browser-based tests.

Explicit exclude patterns in the coverage include/exclude config:

src/**/*.svelte and src/**/*.svelte.ts — correct scope for browser tests
Excluding paraglide/**, generated/**, hooks/** — all correct, these are generated or framework code
src/lib/server/** excluded from the test exclude list — prevents server-only code from running in browser context

Sequential && script composition — eliminates the ENOTEMPTY race documented in the PR. Clean.

if: always() on artifact upload — I can inspect coverage reports on failing builds. Critical for debugging why the gate failed.

Coverage artifact upload — having coverage/client/lcov.info and coverage/server/lcov.info both in the artifact enables future integration with coverage diff tools or PR comments without further CI changes.

## 🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** ### What I checked - Coverage threshold completeness and accuracy - Test file inclusion/exclusion patterns - CI gate behavior with intentionally failing threshold - Quality gate strategy --- ### Blockers **1. Only `branches: 80` threshold — coverage quality gap** `vitest.client-coverage.config.ts` only asserts on branch coverage: ```ts thresholds: { branches: 80 } ``` This means a Svelte component with 80% branch coverage but 5% line coverage passes the gate. For a browser component test suite, `lines` and `functions` are the most meaningful coverage dimensions (you want to know: is the component's render logic actually executed?). Recommend aligning thresholds across all four dimensions at the same 80% floor: ```ts thresholds: { lines: 80, functions: 80, branches: 80, statements: 80 } ``` **2. CI gate fails at ~53% client coverage — every merge to main will have a red CI step** The PR explicitly documents this as "hard gate from day one." I understand and respect the intent: creating immediate pressure to raise coverage rather than deferring it. But the practical consequence is that CI shows a red step on every PR until coverage reaches 80%. This can normalize CI red states and make it harder to notice _new_ failures. Suggestion: Either (a) accept this and treat the coverage step as a known-failing gate with a tracked issue, or (b) start the threshold at the current baseline (~53%) and ratchet it up each sprint. Either approach is valid — I just want it explicitly decided, not accidentally tolerated. --- ### What's done well **`requireAssertions: true`** on the browser project config — excellent. This catches async test patterns where the expect never runs (the test body completes before the assertion fires). This is a common pitfall in browser-based tests. **Explicit exclude patterns** in the coverage `include`/`exclude` config: - `src/**/*.svelte` and `src/**/*.svelte.ts` — correct scope for browser tests - Excluding `paraglide/**`, `generated/**`, `hooks/**` — all correct, these are generated or framework code - `src/lib/server/**` excluded from the test `exclude` list — prevents server-only code from running in browser context **Sequential `&&` script composition** — eliminates the ENOTEMPTY race documented in the PR. Clean. **`if: always()` on artifact upload** — I can inspect coverage reports on failing builds. Critical for debugging why the gate failed. **Coverage artifact upload** — having `coverage/client/lcov.info` and `coverage/server/lcov.info` both in the artifact enables future integration with coverage diff tools or PR comments without further CI changes.

marcel commented

2026-05-09 18:47:28 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

Alignment with the stated goal of issue #425
Completeness of acceptance criteria from the PR description
Scope boundaries (what this PR does and doesn't address)
Requirement traceability

Requirement coverage

The PR description presents a clear test plan with five checkpoints:

Acceptance criterion	Status in PR
`npm test` — 190 files, 1850 tests, all green	Documented ✓
`npm run test:coverage` produces both lcov files	Documented ✓
`coverage/client/lcov.info` references `.svelte` files	Documented ✓
Server threshold ≥ 80% branches → passes	Documented ✓ (~94%)
Client threshold < 80% branches → exits non-zero	Documented ✓ (intentional failing gate)

This is well-specified. The PR knows what it promises and what it doesn't.

One open question worth tracking

The PR description references "Theme 1" (all Svelte components included in coverage scope) and "Theme 3" (hard gate from day one). These appear to be decisions made in issue #425 or during planning — but they are not traceable to the issue body visible in this PR.

If "Theme 1" means "include all Svelte components even if untested, accepting a low starting coverage," that's a significant scope decision that shapes the 53% baseline. If it means "exclude generated and library code," that's less impactful. Worth verifying that the current include/exclude patterns in vitest.client-coverage.config.ts match the agreed scope.

Scope is appropriately bounded

This PR does not attempt to raise coverage — it only makes it measurable. That's the right decomposition. The gap between 53% and 80% is a separate work item, and the failing gate creates the correct incentive to address it without blocking this PR's value from landing.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked - Alignment with the stated goal of issue #425 - Completeness of acceptance criteria from the PR description - Scope boundaries (what this PR does and doesn't address) - Requirement traceability --- ### Requirement coverage The PR description presents a clear test plan with five checkpoints: | Acceptance criterion | Status in PR | |---|---| | `npm test` — 190 files, 1850 tests, all green | Documented ✓ | | `npm run test:coverage` produces both lcov files | Documented ✓ | | `coverage/client/lcov.info` references `.svelte` files | Documented ✓ | | Server threshold ≥ 80% branches → passes | Documented ✓ (~94%) | | Client threshold < 80% branches → exits non-zero | Documented ✓ (intentional failing gate) | This is well-specified. The PR knows what it promises and what it doesn't. --- ### One open question worth tracking The PR description references "Theme 1" (all Svelte components included in coverage scope) and "Theme 3" (hard gate from day one). These appear to be decisions made in issue #425 or during planning — but they are not traceable to the issue body visible in this PR. If "Theme 1" means "include all Svelte components even if untested, accepting a low starting coverage," that's a significant scope decision that shapes the 53% baseline. If it means "exclude generated and library code," that's less impactful. Worth verifying that the current `include`/`exclude` patterns in `vitest.client-coverage.config.ts` match the agreed scope. --- ### Scope is appropriately bounded This PR does not attempt to raise coverage — it only makes it measurable. That's the right decomposition. The gap between 53% and 80% is a separate work item, and the failing gate creates the correct incentive to address it without blocking this PR's value from landing.

marcel commented

2026-05-09 18:47:34 +02:00

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead

Verdict: ✅ Approved

What I checked

UI changes: none
Frontend component changes: none
Svelte files changed: none

This PR is pure test infrastructure — Vitest configuration, CI workflow, and package dependencies. No Svelte components, no UI styling, no user-facing changes.

Nothing in my domain to flag.

The only tangentially UX-relevant note: the coverage include pattern correctly targets src/**/*.svelte and src/**/*.svelte.ts. This means Svelte components — the files I care about most — are now in scope for browser coverage measurement. That's the right foundation for eventually requiring tested states in components (loading, empty, error). Good to have this infrastructure in place.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead **Verdict: ✅ Approved** ### What I checked - UI changes: none - Frontend component changes: none - Svelte files changed: none --- This PR is pure test infrastructure — Vitest configuration, CI workflow, and package dependencies. No Svelte components, no UI styling, no user-facing changes. Nothing in my domain to flag. The only tangentially UX-relevant note: the coverage include pattern correctly targets `src/**/*.svelte` and `src/**/*.svelte.ts`. This means Svelte components — the files I care about most — are now in scope for browser coverage measurement. That's the right foundation for eventually requiring tested states in components (loading, empty, error). Good to have this infrastructure in place.

marcel added 1 commit 2026-05-09 18:57:44 +02:00

fix(test): extend coverage thresholds to all four dimensions

CI / Unit & Component Tests (pull_request) Failing after 6m18s

Details

CI / OCR Service Tests (pull_request) Successful in 43s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m24s

Details

CI / Unit & Component Tests (push) Failing after 6m10s

Details

CI / OCR Service Tests (push) Successful in 32s

Details

CI / Backend Unit Tests (push) Failing after 3m22s

Details

80ccc0f3c6

Add lines, functions, and statements at 80% alongside branches in both
the server (vite.config.ts) and client (vitest.client-coverage.config.ts)
coverage gates — branch-only thresholds allow misleadingly sparse tests to
pass the gate.

Also adds a plugin-sync comment to vitest.client-coverage.config.ts listing
the four Vite plugins mirrored from vite.config.ts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-05-09 18:58:02 +02:00

Review concerns addressed — commit `80ccc0f3`

Felix Brandt (blocker) + Sara Holt (Blocker 1) ✅

Both coverage configs only gated branches: 80, allowing misleadingly sparse tests to pass.

Extended thresholds to all four dimensions at the same 80% floor in both configs:

thresholds: {
  lines: 80,
  functions: 80,
  branches: 80,
  statements: 80
}

Applied to:

frontend/vitest.client-coverage.config.ts (client/Istanbul)
frontend/vite.config.ts (server/v8)

Felix Brandt (non-blocking suggestion) ✅

Added a plugin-sync comment to vitest.client-coverage.config.ts header:

// Plugins mirrored from vite.config.ts: tailwindcss, sveltekit, devtoolsJson, paraglideVitePlugin
// Update here whenever vite.config.ts plugins change.

Sara Holt (Blocker 2) — no code change needed ✅

The failing CI gate (~53% client coverage) is explicitly documented in the PR description as "hard gate from day one / Theme 3." Already an explicit decision, not accidental tolerance.

Tobias Wendt (observation) + Nora Steiner (audit suggestion) — informational ✅

No code changes required. Playwright cache and npm audit are operational concerns, not PR blockers.

🤖 Generated with Claude Code

## Review concerns addressed — commit `80ccc0f3` ### Felix Brandt (blocker) + Sara Holt (Blocker 1) ✅ **Both coverage configs only gated `branches: 80`, allowing misleadingly sparse tests to pass.** Extended thresholds to all four dimensions at the same 80% floor in both configs: ```ts thresholds: { lines: 80, functions: 80, branches: 80, statements: 80 } ``` Applied to: - `frontend/vitest.client-coverage.config.ts` (client/Istanbul) - `frontend/vite.config.ts` (server/v8) ### Felix Brandt (non-blocking suggestion) ✅ Added a plugin-sync comment to `vitest.client-coverage.config.ts` header: ```ts // Plugins mirrored from vite.config.ts: tailwindcss, sveltekit, devtoolsJson, paraglideVitePlugin // Update here whenever vite.config.ts plugins change. ``` ### Sara Holt (Blocker 2) — no code change needed ✅ The failing CI gate (~53% client coverage) is explicitly documented in the PR description as "hard gate from day one / Theme 3." Already an explicit decision, not accidental tolerance. ### Tobias Wendt (observation) + Nora Steiner (audit suggestion) — informational ✅ No code changes required. Playwright cache and `npm audit` are operational concerns, not PR blockers. 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel merged commit 80ccc0f3c6 into main

2026-05-09 19:04:28 +02:00

marcel deleted branch feat/issue-425-browser-coverage

2026-05-09 19:04:29 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#495

fix(test): make browser-project tests contribute to coverage measurement #495

Summary

Coverage numbers at merge

Test plan

👷 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

What I checked

What's done well

Minor observation

One heads-up on the failing gate

👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer

What I checked

Blocker

Suggestions (non-blocking)

🏛️ Markus Keller (@mkeller) — Senior Application Architect

What I checked

Documentation update matrix

Architectural assessment

No ADR needed

🔒 Nora "NullX" Steiner — Application Security Engineer

What I checked

No security issues found

One minor observation (not a vulnerability)

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

What I checked

Blockers

What's done well

📋 Elicit — Requirements Engineer

What I checked

Requirement coverage

One open question worth tracking

Scope is appropriately bounded

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead

What I checked

Review concerns addressed — commit 80ccc0f3

Felix Brandt (blocker) + Sara Holt (Blocker 1) ✅

Felix Brandt (non-blocking suggestion) ✅

Sara Holt (Blocker 2) — no code change needed ✅

Tobias Wendt (observation) + Nora Steiner (audit suggestion) — informational ✅

Review concerns addressed — commit `80ccc0f3`