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

New Issue

marcel · 2026-05-05T15:29:57+02:00

marcel commented

2026-05-05 15:29:57 +02:00

Problem

Coverage is currently only measured against the server project (vitest run --coverage --project=server). The client (browser/Playwright) project — which tests all Svelte components via vitest-browser-svelte — contributes nothing to the coverage number.

This means the 80% branch threshold is gating a narrow slice of the codebase (shared utilities, server helpers, document utilities) while the bulk of the application code (every .svelte component and its companion .ts state files) is entirely invisible to the coverage tool.

Running npm run test -- --coverage across both projects consistently fails to produce a report due to a race condition: both the client and server processes attempt to clean up the same coverage/.tmp directory simultaneously, and one of them fails with ENOTEMPTY. No coverage table is printed and no coverage-summary.json is written.

Goal

Browser-project tests should contribute to the combined coverage report. The 80% threshold should gate the full application — not just server-side utilities.

Investigation paths

1. Fix the `.tmp` race condition

Vitest supports per-project coverage directories. Setting coverage.reportsDirectory to a unique path per project (e.g. coverage/server and coverage/client) would prevent the cleanup conflict. Coverage can then be merged via lcov-result-merger or a similar tool.

2. Enable browser coverage via Istanbul

The v8 provider cannot instrument code running inside Chromium. Vitest browser mode supports Istanbul-based coverage when configured on the browser project:

// in the client project config
{
  test: {
    name: 'client',
    browser: { ... },
    coverage: {
      provider: 'istanbul',
      reportsDirectory: 'coverage/client',
    }
  }
}

This requires @vitest/coverage-istanbul as a dev dependency. Istanbul injects instrumentation at transpile time, so it works inside the browser sandbox.

3. Run projects sequentially

Adding --sequence.concurrent=false to the coverage run forces projects to run one after the other, eliminating the .tmp race condition without requiring separate directories. This is the lowest-effort fix but doubles the runtime.

4. Use a single merged threshold

Once both projects produce reports, the combined lcov.info files can be merged and a single threshold applied to the union. This gives a true picture of application coverage.

Acceptance criteria

npm run test:coverage (or a new test:coverage:full script) produces a combined coverage report covering both server-side TypeScript utilities and browser-rendered Svelte components
The 80% branch threshold applies to the combined report
The race condition (ENOTEMPTY on coverage/.tmp) no longer occurs

PR #422 (domain restructuring) surfaced this gap — test:coverage has always been --project=server
#423 (pre-existing test failures)
Epic #406 (frontend domain packaging)

## Problem Coverage is currently only measured against the `server` project (`vitest run --coverage --project=server`). The `client` (browser/Playwright) project — which tests all Svelte components via `vitest-browser-svelte` — contributes nothing to the coverage number. This means the 80% branch threshold is gating a narrow slice of the codebase (shared utilities, server helpers, document utilities) while the bulk of the application code (every `.svelte` component and its companion `.ts` state files) is entirely invisible to the coverage tool. Running `npm run test -- --coverage` across both projects consistently fails to produce a report due to a race condition: both the `client` and `server` processes attempt to clean up the same `coverage/.tmp` directory simultaneously, and one of them fails with `ENOTEMPTY`. No coverage table is printed and no `coverage-summary.json` is written. ## Goal Browser-project tests should contribute to the combined coverage report. The 80% threshold should gate the full application — not just server-side utilities. ## Investigation paths ### 1. Fix the `.tmp` race condition Vitest supports per-project coverage directories. Setting `coverage.reportsDirectory` to a unique path per project (e.g. `coverage/server` and `coverage/client`) would prevent the cleanup conflict. Coverage can then be merged via `lcov-result-merger` or a similar tool. ### 2. Enable browser coverage via Istanbul The v8 provider cannot instrument code running inside Chromium. Vitest browser mode supports Istanbul-based coverage when configured on the browser project: ```typescript // in the client project config { test: { name: 'client', browser: { ... }, coverage: { provider: 'istanbul', reportsDirectory: 'coverage/client', } } } ``` This requires `@vitest/coverage-istanbul` as a dev dependency. Istanbul injects instrumentation at transpile time, so it works inside the browser sandbox. ### 3. Run projects sequentially Adding `--sequence.concurrent=false` to the coverage run forces projects to run one after the other, eliminating the `.tmp` race condition without requiring separate directories. This is the lowest-effort fix but doubles the runtime. ### 4. Use a single merged threshold Once both projects produce reports, the combined `lcov.info` files can be merged and a single threshold applied to the union. This gives a true picture of application coverage. ## Acceptance criteria - `npm run test:coverage` (or a new `test:coverage:full` script) produces a combined coverage report covering both server-side TypeScript utilities and browser-rendered Svelte components - The 80% branch threshold applies to the combined report - The race condition (`ENOTEMPTY` on `coverage/.tmp`) no longer occurs ## Related - PR #422 (domain restructuring) surfaced this gap — `test:coverage` has always been `--project=server` - #423 (pre-existing test failures) - Epic #406 (frontend domain packaging)

marcel added the P2-medium devops test labels 2026-05-05 15:30:04 +02:00

marcel commented

2026-05-07 17:47:44 +02:00

🏗️ Markus Keller — Application Architect

Observations

The root cause is a design gap, not a bug: the top-level coverage block in vite.config.ts applies only to the server project because the client project has no independent coverage configuration and shares the same coverage/.tmp scratch directory. Both projects racing to clean the same temp directory is the predictable outcome.
The include list in the coverage block (src/lib/shared/utils/**, src/lib/shared/server/**, src/lib/shared/discussion/**, src/lib/document/**) is intentionally narrow — good discipline. But it means the 80% branch threshold is calibrated to a subset of the codebase and gives no signal about Svelte component behaviour.
Approach 3 (sequential projects via --sequence.concurrent=false) solves the race condition with zero new dependencies. However it doubles CI coverage time; for a solo developer on a NAS runner this is a legitimate concern.
Approach 2 (Istanbul in the browser project) is the architecturally correct solution because it actually instruments the code that matters (Svelte components) rather than papering over the race. Istanbul's transpile-time instrumentation is the only mechanism that works inside Chromium's sandbox.
Approach 4 (merged lcov) is separable — it can follow from either approach 1 or 2.

Recommendations

Add a separate coverage.reportsDirectory per project (coverage/server and coverage/client) as the first step. This eliminates the race without any sequencing overhead and costs one config line.
Add @vitest/coverage-istanbul as a dev dependency and configure it on the client project block. The v8 provider is silently a no-op inside Chromium; Istanbul is the supported mechanism for browser-mode coverage. The issue description already identifies this correctly.
Do not use --sequence.concurrent=false as the final answer — it is an acceptable workaround while the Istanbul integration is being configured, but should not become the permanent solution. Sequential projects hide the root cause and penalise CI runtime indefinitely.
Hold off on merging coverage reports with lcov-result-merger until both projects produce valid reports. Merge tooling should be added only when there is data worth merging.
The 80% threshold should remain in place. Once both projects contribute, expect the initial combined number to be lower than 80% until component test coverage catches up — plan for that regression before enforcing the gate on client coverage.

Open Decisions

Which files from the client project should be included in the coverage include list? The current list only covers utility and server code. If all .svelte components are included, the bar will start much lower than 80%. Consider a phased rollout: add components to the include list incrementally as their test coverage rises, rather than gating the entire component layer on day one.
Should a combined lcov threshold gate CI, or should server and client thresholds be gated independently? Independent gates make it clear which project regressed; a combined gate is simpler to configure but obscures the source of a drop.

## 🏗️ Markus Keller — Application Architect ### Observations - The root cause is a design gap, not a bug: the top-level `coverage` block in `vite.config.ts` applies only to the `server` project because the `client` project has no independent `coverage` configuration and shares the same `coverage/.tmp` scratch directory. Both projects racing to clean the same temp directory is the predictable outcome. - The `include` list in the coverage block (`src/lib/shared/utils/**`, `src/lib/shared/server/**`, `src/lib/shared/discussion/**`, `src/lib/document/**`) is intentionally narrow — good discipline. But it means the 80% branch threshold is calibrated to a subset of the codebase and gives no signal about Svelte component behaviour. - Approach 3 (sequential projects via `--sequence.concurrent=false`) solves the race condition with zero new dependencies. However it doubles CI coverage time; for a solo developer on a NAS runner this is a legitimate concern. - Approach 2 (Istanbul in the browser project) is the architecturally correct solution because it actually instruments the code that matters (Svelte components) rather than papering over the race. Istanbul's transpile-time instrumentation is the only mechanism that works inside Chromium's sandbox. - Approach 4 (merged lcov) is separable — it can follow from either approach 1 or 2. ### Recommendations - **Add a separate `coverage.reportsDirectory` per project** (`coverage/server` and `coverage/client`) as the first step. This eliminates the race without any sequencing overhead and costs one config line. - **Add `@vitest/coverage-istanbul` as a dev dependency and configure it on the `client` project block.** The v8 provider is silently a no-op inside Chromium; Istanbul is the supported mechanism for browser-mode coverage. The issue description already identifies this correctly. - **Do not use `--sequence.concurrent=false` as the final answer** — it is an acceptable workaround while the Istanbul integration is being configured, but should not become the permanent solution. Sequential projects hide the root cause and penalise CI runtime indefinitely. - **Hold off on merging coverage reports with `lcov-result-merger` until both projects produce valid reports.** Merge tooling should be added only when there is data worth merging. - The 80% threshold should remain in place. Once both projects contribute, expect the initial combined number to be lower than 80% until component test coverage catches up — plan for that regression before enforcing the gate on `client` coverage. ### Open Decisions - **Which files from the `client` project should be included in the coverage `include` list?** The current list only covers utility and server code. If all `.svelte` components are included, the bar will start much lower than 80%. Consider a phased rollout: add components to the include list incrementally as their test coverage rises, rather than gating the entire component layer on day one. - **Should a combined lcov threshold gate CI, or should server and client thresholds be gated independently?** Independent gates make it clear which project regressed; a combined gate is simpler to configure but obscures the source of a drop.

marcel commented

2026-05-07 17:48:02 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

I looked at vite.config.ts. The coverage block lives at the top-level test object, which means it is inherited by both projects via extends: './vite.config.ts'. The client project adds browser config but no coverage override — so both projects use the same provider: 'v8' and the same reportsDirectory (default coverage/). The race on coverage/.tmp is structural.
The test:coverage script is vitest run --coverage --project=server. This explicitly runs only the server project, which is why the race never manifests during the normal coverage workflow. The issue title says "browser-project tests contribute nothing to coverage measurement" — that's exactly the consequence of --project=server.
@vitest/coverage-v8 is already in devDependencies. @vitest/coverage-istanbul is not. Adding it is a one-line package.json change plus installing it.
The exclude: ['**/*.svelte', '**/*.svelte.ts', '**/__mocks__/**'] in the coverage block explicitly excludes Svelte files from the server project's coverage — which is correct, since those files cannot be instrumented by v8 in a Node environment. The browser project, with Istanbul, would not need this exclusion.
The component test files already follow the *.svelte.spec.ts naming pattern and are routed to the client project via include: ['src/**/*.svelte.{test,spec}.{js,ts}']. The test infrastructure is ready; only the coverage instrumentation is missing.

Recommendations

Red phase first: before touching vite.config.ts, write a test that asserts the expected coverage report file exists at coverage/client/lcov.info after running the browser project. This gives you a concrete failing signal.

Minimum viable change for the client project block:

{
  extends: './vite.config.ts',
  test: {
    name: 'client',
    browser: { /* existing config */ },
    include: ['src/**/*.svelte.{test,spec}.{js,ts}'],
    exclude: ['src/lib/server/**'],
    coverage: {
      provider: 'istanbul',
      reportsDirectory: 'coverage/client',
      include: ['src/lib/**/*.svelte', 'src/lib/**/*.svelte.ts'],
      exclude: ['src/lib/**/__mocks__/**']
    }
  }
}

This is the smallest diff that fixes the race and enables browser coverage.

Update test:coverage in package.json to run both projects and then merge:
```
"test:coverage": "vitest run --coverage --project=server && vitest run --coverage --project=client"
```
Sequencing with && prevents the race entirely without --sequence.concurrent=false.
Install @vitest/coverage-istanbul — do not install it as @vitest/coverage-v8 replacement; both should coexist since the server project continues using v8.
Do not delete the explicit --project=server guard from the current test:coverage script until the Istanbul setup is confirmed to produce a valid report. Keep the old script as test:coverage:server for regression testing during the migration.

Open Decisions

Should coverage/client/ be added to .gitignore, or is there a reason to commit coverage artifacts? (Current coverage/ directory appears to already be ignored — worth verifying.)

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations - I looked at `vite.config.ts`. The `coverage` block lives at the top-level `test` object, which means it is inherited by both projects via `extends: './vite.config.ts'`. The `client` project adds browser config but no `coverage` override — so both projects use the same `provider: 'v8'` and the same `reportsDirectory` (default `coverage/`). The race on `coverage/.tmp` is structural. - The `test:coverage` script is `vitest run --coverage --project=server`. This explicitly runs only the `server` project, which is why the race never manifests during the normal coverage workflow. The issue title says "browser-project tests contribute nothing to coverage measurement" — that's exactly the consequence of `--project=server`. - `@vitest/coverage-v8` is already in `devDependencies`. `@vitest/coverage-istanbul` is not. Adding it is a one-line `package.json` change plus installing it. - The `exclude: ['**/*.svelte', '**/*.svelte.ts', '**/__mocks__/**']` in the coverage block explicitly excludes Svelte files from the server project's coverage — which is correct, since those files cannot be instrumented by v8 in a Node environment. The browser project, with Istanbul, would not need this exclusion. - The component test files already follow the `*.svelte.spec.ts` naming pattern and are routed to the `client` project via `include: ['src/**/*.svelte.{test,spec}.{js,ts}']`. The test infrastructure is ready; only the coverage instrumentation is missing. ### Recommendations - **Red phase first:** before touching `vite.config.ts`, write a test that asserts the expected coverage report file exists at `coverage/client/lcov.info` after running the browser project. This gives you a concrete failing signal. - **Minimum viable change for the `client` project block:** ```typescript { extends: './vite.config.ts', test: { name: 'client', browser: { /* existing config */ }, include: ['src/**/*.svelte.{test,spec}.{js,ts}'], exclude: ['src/lib/server/**'], coverage: { provider: 'istanbul', reportsDirectory: 'coverage/client', include: ['src/lib/**/*.svelte', 'src/lib/**/*.svelte.ts'], exclude: ['src/lib/**/__mocks__/**'] } } } ``` This is the smallest diff that fixes the race and enables browser coverage. - **Update `test:coverage` in `package.json`** to run both projects and then merge: ``` "test:coverage": "vitest run --coverage --project=server && vitest run --coverage --project=client" ``` Sequencing with `&&` prevents the race entirely without `--sequence.concurrent=false`. - **Install `@vitest/coverage-istanbul`** — do not install it as `@vitest/coverage-v8` replacement; both should coexist since the server project continues using v8. - **Do not delete the explicit `--project=server` guard** from the current `test:coverage` script until the Istanbul setup is confirmed to produce a valid report. Keep the old script as `test:coverage:server` for regression testing during the migration. ### Open Decisions - Should `coverage/client/` be added to `.gitignore`, or is there a reason to commit coverage artifacts? (Current `coverage/` directory appears to already be ignored — worth verifying.)

marcel commented

2026-05-07 17:48:22 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

The CI workflow (.gitea/workflows/ci.yml) runs npm test (which maps to vitest run across both projects) but does not run npm run test:coverage. Coverage is therefore not gated in CI at all today — the 80% branch threshold only applies if someone manually runs test:coverage locally. This is the deeper problem the issue exposes.
The unit-tests job uses mcr.microsoft.com/playwright:v1.58.2-noble as the container image, which is sensible — Playwright and Chromium are pre-installed. This image will also support Istanbul-instrumented browser coverage without any additional system-level dependencies.
The package-lock.json is currently untracked (visible in git status at the start of this session as ?? package-lock.json). Without a committed lock file the npm ci step in CI is effectively npm install with version drift risk. That should be fixed regardless of this issue.
The issue's Approach 3 (--sequence.concurrent=false) would add wall-clock time to the unit-tests CI job. At current test scale this is probably 30–60 seconds extra, acceptable but not free.
actions/upload-artifact@v4 is already being used — good, no deprecated action concerns here.
There is no dedicated coverage-upload step in CI. Even if coverage is generated locally, it is never persisted as a CI artifact.

Recommendations

Add a test:coverage step to the unit-tests CI job immediately after the test run step. Even before the browser project is fixed, having server coverage gated in CI is better than the current situation (no gate):

- name: Run coverage (server)
  run: npm run test:coverage
  working-directory: frontend

- name: Upload coverage report
  if: always()
  uses: actions/upload-artifact@v4
  with:
    name: coverage-report
    path: frontend/coverage/

Commit package-lock.json — this is a prerequisite for reliable npm ci in CI. The untracked status is a latent reliability risk unrelated to but more urgent than this issue.
Do not use --sequence.concurrent=false as a CI strategy. It serialises all projects globally; adding a third project later doubles the pain again. Separate reportsDirectory values solve the race structurally.

After Istanbul is configured, update the CI job to run both coverage commands sequentially and upload both report directories as separate artifacts:

- name: Run coverage (server + client)
  run: |
    npm run test:coverage -- --project=server
    npm run test:coverage -- --project=client
  working-directory: frontend

Add a threshold-failure check as a blocking step, not just an artifact upload. vitest run --coverage exits non-zero when thresholds are not met — make sure the CI step is not wrapped in if: always() which would mask the failure.

Open Decisions

Should the merged lcov.info be uploaded to a code coverage service (Codecov, Coveralls, SonarQube)? Given the self-hosted philosophy, a local HTML report artifact is sufficient — but worth deciding before building the merge pipeline.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - The CI workflow (`.gitea/workflows/ci.yml`) runs `npm test` (which maps to `vitest run` across both projects) but does **not** run `npm run test:coverage`. Coverage is therefore not gated in CI at all today — the 80% branch threshold only applies if someone manually runs `test:coverage` locally. This is the deeper problem the issue exposes. - The `unit-tests` job uses `mcr.microsoft.com/playwright:v1.58.2-noble` as the container image, which is sensible — Playwright and Chromium are pre-installed. This image will also support Istanbul-instrumented browser coverage without any additional system-level dependencies. - The `package-lock.json` is currently untracked (visible in `git status` at the start of this session as `?? package-lock.json`). Without a committed lock file the `npm ci` step in CI is effectively `npm install` with version drift risk. That should be fixed regardless of this issue. - The issue's Approach 3 (`--sequence.concurrent=false`) would add wall-clock time to the `unit-tests` CI job. At current test scale this is probably 30–60 seconds extra, acceptable but not free. - `actions/upload-artifact@v4` is already being used — good, no deprecated action concerns here. - There is no dedicated coverage-upload step in CI. Even if coverage is generated locally, it is never persisted as a CI artifact. ### Recommendations - **Add a `test:coverage` step to the `unit-tests` CI job** immediately after the test run step. Even before the browser project is fixed, having server coverage gated in CI is better than the current situation (no gate): ```yaml - name: Run coverage (server) run: npm run test:coverage working-directory: frontend - name: Upload coverage report if: always() uses: actions/upload-artifact@v4 with: name: coverage-report path: frontend/coverage/ ``` - **Commit `package-lock.json`** — this is a prerequisite for reliable `npm ci` in CI. The untracked status is a latent reliability risk unrelated to but more urgent than this issue. - **Do not use `--sequence.concurrent=false`** as a CI strategy. It serialises all projects globally; adding a third project later doubles the pain again. Separate `reportsDirectory` values solve the race structurally. - **After Istanbul is configured**, update the CI job to run both coverage commands sequentially and upload both report directories as separate artifacts: ```yaml - name: Run coverage (server + client) run: | npm run test:coverage -- --project=server npm run test:coverage -- --project=client working-directory: frontend ``` - **Add a threshold-failure check as a blocking step**, not just an artifact upload. `vitest run --coverage` exits non-zero when thresholds are not met — make sure the CI step is not wrapped in `if: always()` which would mask the failure. ### Open Decisions - Should the merged `lcov.info` be uploaded to a code coverage service (Codecov, Coveralls, SonarQube)? Given the self-hosted philosophy, a local HTML report artifact is sufficient — but worth deciding before building the merge pipeline.

marcel commented

2026-05-07 17:48:36 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue has a clear problem statement and four well-labelled investigation paths. The acceptance criteria are specific and testable — this is a well-written issue.
One ambiguity in the acceptance criteria: "npm run test:coverage (or a new test:coverage:full script)" — the parenthetical "or" means the final script name is undecided. Any implementation will need to choose one, and the issue should specify which.
The 80% branch threshold is stated as the gate for "the combined report," but the issue does not define what "combined" means precisely: does it mean a single merged lcov, or two separate thresholds that must both pass? These have different implementation paths and different failure modes.
The issue references #422 (domain restructuring) and #423 (pre-existing test failures) as related. If pre-existing test failures (#423) are not resolved before this issue is implemented, the initial combined coverage number may not be meaningful. The dependency is noted but not enforced as a blocking prerequisite.
There is no non-functional requirement stated for CI runtime impact. Approach 3 (sequential) is explicitly flagged as "doubles the runtime," which suggests runtime is a concern — but no acceptable maximum is stated.
The acceptance criteria do not cover what happens when one project's threshold passes and the other's fails. Is a partial failure acceptable during a transition period?

Recommendations

Resolve the script name ambiguity before implementation starts. Recommend test:coverage:full as the new script (leaving test:coverage pointing at the existing server-only command for backward compatibility during migration), then rename once browser coverage is stable.
Add an explicit prerequisite link to #423 in the issue body. If pre-existing failures inflate the "missed branches" count, the 80% threshold will fail for reasons unrelated to this issue.
Clarify the threshold model in the acceptance criteria: "The 80% threshold applies to the combined report" should specify whether this means a single merged file or two independent thresholds. Recommended: two independent thresholds (server stays at 80%, client starts at whatever the initial measurement shows, with a roadmap to raise it to 80%).
Add a CI runtime NFR: "The combined coverage run shall complete within X minutes in the unit-tests CI job." Without this, the sequential approach (Approach 3) could be accepted even if it adds 5 minutes to every PR.

Open Decisions

What is the acceptable initial branch coverage percentage for the client project on day one of measurement? Starting at 80% may be unachievable without a large test-writing effort. Setting a realistic baseline (whatever the first measurement shows) and ratcheting upward avoids a situation where the gate is immediately broken by the act of enabling it.

## 📋 Elicit — Requirements Engineer ### Observations - The issue has a clear problem statement and four well-labelled investigation paths. The acceptance criteria are specific and testable — this is a well-written issue. - One ambiguity in the acceptance criteria: "`npm run test:coverage` (or a new `test:coverage:full` script)" — the parenthetical "or" means the final script name is undecided. Any implementation will need to choose one, and the issue should specify which. - The 80% branch threshold is stated as the gate for "the combined report," but the issue does not define what "combined" means precisely: does it mean a single merged lcov, or two separate thresholds that must both pass? These have different implementation paths and different failure modes. - The issue references #422 (domain restructuring) and #423 (pre-existing test failures) as related. If pre-existing test failures (#423) are not resolved before this issue is implemented, the initial combined coverage number may not be meaningful. The dependency is noted but not enforced as a blocking prerequisite. - There is no non-functional requirement stated for CI runtime impact. Approach 3 (sequential) is explicitly flagged as "doubles the runtime," which suggests runtime is a concern — but no acceptable maximum is stated. - The acceptance criteria do not cover what happens when one project's threshold passes and the other's fails. Is a partial failure acceptable during a transition period? ### Recommendations - **Resolve the script name ambiguity before implementation starts.** Recommend `test:coverage:full` as the new script (leaving `test:coverage` pointing at the existing server-only command for backward compatibility during migration), then rename once browser coverage is stable. - **Add an explicit prerequisite link to #423** in the issue body. If pre-existing failures inflate the "missed branches" count, the 80% threshold will fail for reasons unrelated to this issue. - **Clarify the threshold model in the acceptance criteria:** "The 80% threshold applies to the combined report" should specify whether this means a single merged file or two independent thresholds. Recommended: two independent thresholds (server stays at 80%, client starts at whatever the initial measurement shows, with a roadmap to raise it to 80%). - **Add a CI runtime NFR:** "The combined coverage run shall complete within X minutes in the `unit-tests` CI job." Without this, the sequential approach (Approach 3) could be accepted even if it adds 5 minutes to every PR. ### Open Decisions - What is the acceptable initial branch coverage percentage for the `client` project on day one of measurement? Starting at 80% may be unachievable without a large test-writing effort. Setting a realistic baseline (whatever the first measurement shows) and ratcheting upward avoids a situation where the gate is immediately broken by the act of enabling it.

marcel commented

2026-05-07 17:48:54 +02:00

🔒 Nora Steiner (NullX) — Security Engineer

Observations

This issue is primarily a testing infrastructure concern, not a security vulnerability. That said, there is a security-relevant dimension: coverage gaps in Svelte component tests mean that access-control UI logic is entirely invisible to the coverage tool.
Looking at the codebase, Svelte components render conditional UI based on user permissions (e.g., hiding edit/delete controls for users without WRITE_ALL). The server enforces these permissions via @RequirePermission, but if a component-level conditional is wrong — say, an edit button visible to a user who lacks WRITE_ALL — that is a UX security flaw that only a browser-executed component test can catch. With zero browser coverage measurement, there is no way to know if these branches are tested.
The --project=server filter means that all *.svelte.spec.ts files (which test rendered component state, including conditional UI based on permissions) contribute nothing to the coverage gate. A developer could remove a permission check from a Svelte template and the coverage threshold would not move.
Istanbul transpile-time instrumentation is the correct provider for this use case. v8 coverage relies on the V8 runtime's built-in coverage sampling, which does not work inside Chromium's sandboxed renderer process — this is a known Vitest limitation, not a bug in the project's configuration.
There are no security-specific concerns with lcov-result-merger or @vitest/coverage-istanbul themselves. Both are well-maintained packages with no known CVEs at this time.

Recommendations

Frame the Istanbul addition to the team as a security quality measure, not just a devops task. Component tests that cover conditional permission rendering are security regression tests. Making them visible in the coverage report gives them the same status as backend permission tests.
After enabling browser coverage, add explicit branch coverage for security-sensitive conditionals in Svelte components — specifically: components that conditionally render write actions (edit, delete, upload) based on the user's permission set. These branches should be in the coverage include list from day one.
Do not expose the coverage HTML report at a public URL. If the combined lcov is ever fed into a hosted service, make sure the Gitea secrets handling (${{ secrets.CODECOV_TOKEN }}) follows the same pattern as other CI secrets. Coverage reports can reveal file paths, function names, and dead code that aids attackers in mapping the application surface.
Verify that @vitest/coverage-istanbul does not accidentally instrument src/lib/server/ files in the browser project context. Server-only code (auth cookie handling, session management) should remain in the server project's coverage scope only. The exclude list in the client project's coverage block should explicitly include src/lib/server/** and src/hooks/**.

## 🔒 Nora Steiner (NullX) — Security Engineer ### Observations - This issue is primarily a testing infrastructure concern, not a security vulnerability. That said, there is a security-relevant dimension: **coverage gaps in Svelte component tests mean that access-control UI logic is entirely invisible to the coverage tool.** - Looking at the codebase, Svelte components render conditional UI based on user permissions (e.g., hiding edit/delete controls for users without `WRITE_ALL`). The server enforces these permissions via `@RequirePermission`, but if a component-level conditional is wrong — say, an edit button visible to a user who lacks `WRITE_ALL` — that is a UX security flaw that only a browser-executed component test can catch. With zero browser coverage measurement, there is no way to know if these branches are tested. - The `--project=server` filter means that all `*.svelte.spec.ts` files (which test rendered component state, including conditional UI based on permissions) contribute nothing to the coverage gate. A developer could remove a permission check from a Svelte template and the coverage threshold would not move. - Istanbul transpile-time instrumentation is the correct provider for this use case. v8 coverage relies on the V8 runtime's built-in coverage sampling, which does not work inside Chromium's sandboxed renderer process — this is a known Vitest limitation, not a bug in the project's configuration. - There are no security-specific concerns with `lcov-result-merger` or `@vitest/coverage-istanbul` themselves. Both are well-maintained packages with no known CVEs at this time. ### Recommendations - **Frame the Istanbul addition to the team as a security quality measure, not just a devops task.** Component tests that cover conditional permission rendering are security regression tests. Making them visible in the coverage report gives them the same status as backend permission tests. - **After enabling browser coverage, add explicit branch coverage for security-sensitive conditionals in Svelte components** — specifically: components that conditionally render write actions (edit, delete, upload) based on the user's permission set. These branches should be in the coverage `include` list from day one. - **Do not expose the coverage HTML report at a public URL.** If the combined lcov is ever fed into a hosted service, make sure the Gitea secrets handling (`${{ secrets.CODECOV_TOKEN }}`) follows the same pattern as other CI secrets. Coverage reports can reveal file paths, function names, and dead code that aids attackers in mapping the application surface. - **Verify that `@vitest/coverage-istanbul` does not accidentally instrument `src/lib/server/` files in the browser project context.** Server-only code (auth cookie handling, session management) should remain in the server project's coverage scope only. The `exclude` list in the client project's coverage block should explicitly include `src/lib/server/**` and `src/hooks/**`.

marcel commented

2026-05-07 17:49:22 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The core problem is that the test pyramid has a blind spot: the component layer (browser project, all *.svelte.spec.ts files) exists and runs, but its output is not measured. The 80% branch threshold is therefore gating only the base of the pyramid (shared utilities, server helpers) while the widest coverage gap — Svelte component branches — goes undetected.
Looking at the src/lib/document/ directory, there are component spec files for most major UI components (BulkDocumentEditLayout.svelte.spec.ts, UploadZone.svelte.spec.ts, DocumentRow.svelte.spec.ts, etc.). These tests exist and are running in the client project. The issue is not that coverage is missing — it is that the tool is not measuring it.
The v8 provider silently produces no coverage inside Chromium: it does not throw an error, it simply collects nothing. This is the most dangerous failure mode — the illusion of measurement without the reality.
The race condition (ENOTEMPTY on coverage/.tmp) only manifests when both projects run simultaneously without separate reportsDirectory values. The test:coverage script currently sidesteps this by explicitly targeting only the server project, so the race has likely never been observed in practice on this codebase. The issue is correct that it would surface if someone ran vitest run --coverage across both projects.
There is no flakiness risk specific to Istanbul instrumentation in browser mode. Istanbul injects counters at transpile time (via Babel or SWC transform); those counters are synchronous and deterministic. This is safer than v8's runtime sampling from a test reliability standpoint.

Recommendations

Treat the Istanbul setup as a test pyramid completeness fix, not just a coverage tooling fix. The spec files already exist. The instrumentation is the only missing piece.
After enabling browser coverage, audit which Svelte component branches are currently uncovered and prioritise:
1. Error states (API failure paths) — these are consistently undertested
2. Empty states (lists with zero items)
3. Conditional UI based on user state (logged-in vs. guest, permissions)
4. Loading states
Set the initial client coverage threshold to whatever the first measurement produces, then create a follow-up issue to raise it incrementally. A coverage gate that starts at 40% and ratchets to 80% over several sprints is better than a gate that is immediately broken and bypassed.
Add the coverage step to CI before this issue is closed — the acceptance criteria require test:coverage:full to produce a combined report, but without CI enforcement the gate is advisory only. A quality gate that does not gate is not a gate.
Verify the client project's exclude list mirrors the server project's exclusions for generated code: src/lib/paraglide/**, src/lib/generated/**. Generated files should not count toward or against the coverage threshold.

Open Decisions

Should the client coverage threshold be enforced as a hard CI failure gate from day one, or should it start as a reporting-only metric? Given that the initial number is unknown, starting as a report-only metric for one sprint is pragmatic — but set a calendar date for when it becomes a hard gate, not a "when we get there."

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - The core problem is that the test pyramid has a blind spot: the component layer (browser project, all `*.svelte.spec.ts` files) exists and runs, but its output is not measured. The 80% branch threshold is therefore gating only the base of the pyramid (shared utilities, server helpers) while the widest coverage gap — Svelte component branches — goes undetected. - Looking at the `src/lib/document/` directory, there are component spec files for most major UI components (`BulkDocumentEditLayout.svelte.spec.ts`, `UploadZone.svelte.spec.ts`, `DocumentRow.svelte.spec.ts`, etc.). These tests exist and are running in the `client` project. The issue is not that coverage is missing — it is that the tool is not measuring it. - The v8 provider silently produces no coverage inside Chromium: it does not throw an error, it simply collects nothing. This is the most dangerous failure mode — the illusion of measurement without the reality. - The race condition (`ENOTEMPTY` on `coverage/.tmp`) only manifests when both projects run simultaneously without separate `reportsDirectory` values. The `test:coverage` script currently sidesteps this by explicitly targeting only the server project, so the race has likely never been observed in practice on this codebase. The issue is correct that it would surface if someone ran `vitest run --coverage` across both projects. - There is no flakiness risk specific to Istanbul instrumentation in browser mode. Istanbul injects counters at transpile time (via Babel or SWC transform); those counters are synchronous and deterministic. This is safer than v8's runtime sampling from a test reliability standpoint. ### Recommendations - **Treat the Istanbul setup as a test pyramid completeness fix, not just a coverage tooling fix.** The spec files already exist. The instrumentation is the only missing piece. - **After enabling browser coverage, audit which Svelte component branches are currently uncovered** and prioritise: 1. Error states (API failure paths) — these are consistently undertested 2. Empty states (lists with zero items) 3. Conditional UI based on user state (logged-in vs. guest, permissions) 4. Loading states - **Set the initial `client` coverage threshold to whatever the first measurement produces**, then create a follow-up issue to raise it incrementally. A coverage gate that starts at 40% and ratchets to 80% over several sprints is better than a gate that is immediately broken and bypassed. - **Add the coverage step to CI before this issue is closed** — the acceptance criteria require `test:coverage:full` to produce a combined report, but without CI enforcement the gate is advisory only. A quality gate that does not gate is not a gate. - **Verify the `client` project's `exclude` list mirrors the `server` project's exclusions for generated code:** `src/lib/paraglide/**`, `src/lib/generated/**`. Generated files should not count toward or against the coverage threshold. ### Open Decisions - Should the `client` coverage threshold be enforced as a hard CI failure gate from day one, or should it start as a reporting-only metric? Given that the initial number is unknown, starting as a report-only metric for one sprint is pragmatic — but set a calendar date for when it becomes a hard gate, not a "when we get there."

marcel commented

2026-05-07 17:49:40 +02:00

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Observations

This is an infrastructure issue with no direct UI or accessibility impact. I have no design feedback on the coverage tooling itself.
There is one indirect UX consequence worth naming: the component tests being run (but unmeasured) include tests for accessibility-sensitive UI patterns — focus management, conditional rendering of interactive elements, and ARIA state changes. If these tests are not being measured, there is less incentive to keep them at high coverage, which over time degrades the reliability of the accessibility safety net.
The vitest-browser-svelte tests run in a real Chromium instance, which means they execute against the real DOM including CSS layout and ARIA tree. This makes them uniquely valuable for catching accessibility regressions that JSDOM-based tests miss. Bringing these tests into coverage measurement reinforces their importance.

Recommendations

When the browser coverage include list is defined, make sure it explicitly covers components in src/lib/shared/primitives/ (BackButton, modal dialogs, form fields) — these are the components that touch focus management, ARIA roles, and keyboard navigation. They should be among the first components with meaningful branch coverage.
Once browser coverage is enabled and baseline numbers are established, treat components that have zero coverage on accessibility-critical branches (conditional aria-* attributes, focus trap logic, error state announcement via aria-live) as high-priority test targets — not just for the coverage number, but because those branches represent real user failure modes for keyboard and screen reader users.

No open decisions from a UX perspective — this is firmly an infrastructure call.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist ### Observations - This is an infrastructure issue with no direct UI or accessibility impact. I have no design feedback on the coverage tooling itself. - There is one indirect UX consequence worth naming: the component tests being run (but unmeasured) include tests for accessibility-sensitive UI patterns — focus management, conditional rendering of interactive elements, and ARIA state changes. If these tests are not being measured, there is less incentive to keep them at high coverage, which over time degrades the reliability of the accessibility safety net. - The `vitest-browser-svelte` tests run in a real Chromium instance, which means they execute against the real DOM including CSS layout and ARIA tree. This makes them uniquely valuable for catching accessibility regressions that JSDOM-based tests miss. Bringing these tests into coverage measurement reinforces their importance. ### Recommendations - When the browser coverage `include` list is defined, make sure it explicitly covers components in `src/lib/shared/primitives/` (BackButton, modal dialogs, form fields) — these are the components that touch focus management, ARIA roles, and keyboard navigation. They should be among the first components with meaningful branch coverage. - Once browser coverage is enabled and baseline numbers are established, treat components that have zero coverage on accessibility-critical branches (conditional `aria-*` attributes, focus trap logic, error state announcement via `aria-live`) as high-priority test targets — not just for the coverage number, but because those branches represent real user failure modes for keyboard and screen reader users. _No open decisions from a UX perspective — this is firmly an infrastructure call._

marcel commented

2026-05-07 17:50:28 +02:00

Decision Queue — Cross-Persona Open Items

Items raised across the review that need a human judgment call before or during implementation.

Theme 1: Client coverage `include` scope and initial threshold

Raised by: Markus, Sara, Elicit

The client project's coverage include list needs a scope decision. Two options:

A) Narrow scope first — add only a handful of well-tested components; set the initial threshold at whatever they achieve (likely 60–70%). Ratchet up as test coverage grows. No day-one gate failure.
B) Full component scope — include all .svelte and .svelte.ts files immediately; accept that the gate will fail on day one and fix coverage as a follow-up. More honest picture, more immediate pain.

Recommendation from review: option A. The gate being immediately broken is worse than a lower-but-honest threshold that tightens over time.

Theme 2: Combined vs. independent coverage thresholds

Raised by: Markus, Elicit

Once both server and client projects produce reports, the threshold enforcement can go two ways:

A) Independent thresholds — server must meet 80% branches; client must meet its own separately configured threshold. A regression in one does not hide behind gains in the other.
B) Single merged threshold — lcov-result-merger combines both lcov files; one 80% threshold on the union. Simpler to configure; harder to diagnose which project regressed.

Recommendation from review: option A (independent). The two project types test fundamentally different things; their coverage should be measured and gated separately.

Theme 3: CI enforcement timing for the client gate

Raised by: Sara, Tobias

Should the client coverage threshold be a hard CI failure gate from day one of this issue landing, or a reporting-only metric for an initial period?

A) Hard gate immediately — threshold violation fails the CI job from the first PR that includes this change.
B) Report-only for one sprint — coverage is uploaded as an artifact, threshold violation is a warning only. Becomes a hard gate after the first measurement sprint.

Recommendation from review: option B, but with a fixed date committed in the issue (not "when we get there").

Theme 4: Script naming strategy

Raised by: Elicit

The acceptance criteria list "npm run test:coverage (or a new test:coverage:full script)" as the target. The naming needs to be decided before implementation touches package.json:

A) Rename test:coverage to cover both projects (breaking change for anyone using the old script).
B) Add test:coverage:full as a new script; leave test:coverage pointing at server-only (backward compatible during migration; rename in a follow-up).

Recommendation from review: option B. Backward compatibility during the transition is worth the two-step rename.

Theme 5: Coverage report persistence / hosting

Raised by: Tobias, Nora

Where should the combined coverage report live after CI runs?

A) CI artifact only — uploaded to Gitea Actions artifacts; visible per-run, not persistent. Self-hosted, zero cost, no external service.
B) External coverage service — Codecov, Coveralls, etc. Requires API token as a Gitea secret; not self-hosted.

Recommendation from review: option A. Consistent with the self-hosted philosophy; HTML artifacts in Gitea are sufficient for a solo project.

## Decision Queue — Cross-Persona Open Items Items raised across the review that need a human judgment call before or during implementation. --- ### Theme 1: Client coverage `include` scope and initial threshold **Raised by:** Markus, Sara, Elicit The `client` project's coverage `include` list needs a scope decision. Two options: - **A) Narrow scope first** — add only a handful of well-tested components; set the initial threshold at whatever they achieve (likely 60–70%). Ratchet up as test coverage grows. No day-one gate failure. - **B) Full component scope** — include all `.svelte` and `.svelte.ts` files immediately; accept that the gate will fail on day one and fix coverage as a follow-up. More honest picture, more immediate pain. Recommendation from review: option A. The gate being immediately broken is worse than a lower-but-honest threshold that tightens over time. --- ### Theme 2: Combined vs. independent coverage thresholds **Raised by:** Markus, Elicit Once both server and client projects produce reports, the threshold enforcement can go two ways: - **A) Independent thresholds** — server must meet 80% branches; client must meet its own separately configured threshold. A regression in one does not hide behind gains in the other. - **B) Single merged threshold** — `lcov-result-merger` combines both lcov files; one 80% threshold on the union. Simpler to configure; harder to diagnose which project regressed. Recommendation from review: option A (independent). The two project types test fundamentally different things; their coverage should be measured and gated separately. --- ### Theme 3: CI enforcement timing for the client gate **Raised by:** Sara, Tobias Should the `client` coverage threshold be a hard CI failure gate from day one of this issue landing, or a reporting-only metric for an initial period? - **A) Hard gate immediately** — threshold violation fails the CI job from the first PR that includes this change. - **B) Report-only for one sprint** — coverage is uploaded as an artifact, threshold violation is a warning only. Becomes a hard gate after the first measurement sprint. Recommendation from review: option B, but with a fixed date committed in the issue (not "when we get there"). --- ### Theme 4: Script naming strategy **Raised by:** Elicit The acceptance criteria list "`npm run test:coverage` (or a new `test:coverage:full` script)" as the target. The naming needs to be decided before implementation touches `package.json`: - **A) Rename `test:coverage` to cover both projects** (breaking change for anyone using the old script). - **B) Add `test:coverage:full` as a new script; leave `test:coverage` pointing at server-only** (backward compatible during migration; rename in a follow-up). Recommendation from review: option B. Backward compatibility during the transition is worth the two-step rename. --- ### Theme 5: Coverage report persistence / hosting **Raised by:** Tobias, Nora Where should the combined coverage report live after CI runs? - **A) CI artifact only** — uploaded to Gitea Actions artifacts; visible per-run, not persistent. Self-hosted, zero cost, no external service. - **B) External coverage service** — Codecov, Coveralls, etc. Requires API token as a Gitea secret; not self-hosted. Recommendation from review: option A. Consistent with the self-hosted philosophy; HTML artifacts in Gitea are sufficient for a solo project.

marcel commented

2026-05-09 17:16:47 +02:00

Theme 1: All at once
Theme 2: A (but both 80%)
Theme 3: A Hard gate
Theme 4: A test:coverage
Theme 5: CI Artifact

Theme 1: All at once Theme 2: A (but both 80%) Theme 3: A Hard gate Theme 4: A test:coverage Theme 5: CI Artifact

marcel referenced a pull request that will close this issue

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

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

marcel commented

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

Implementation complete — PR #495

What was built

4 commits on feat/issue-425-browser-coverage:

1a28e311 build(deps): add @vitest/coverage-istanbul for browser-project coverage
bb374bf2 feat(test): add Istanbul browser coverage via standalone client config
16f69fff feat(test): update test:coverage to run both server and client projects
eccecf35 ci: add combined coverage gate to unit-tests job

Key implementation note

Vitest 4 silently ignores coverage overrides placed inside individual test.projects entries — the global coverage block always wins. The fix is a standalone vitest.client-coverage.config.ts where the Istanbul coverage block sits at the root test level and is actually honoured. vite.config.ts retains the v8 block for the server project (coverage/server); the standalone config writes to coverage/client.

Acceptance criteria status

Criterion	Status
`npm run test:coverage` produces `coverage/server/lcov.info` (v8)	✅
`npm run test:coverage` produces `coverage/client/lcov.info` (Istanbul)	✅
`coverage/client/lcov.info` covers `.svelte` components	✅
Race condition (`ENOTEMPTY` on `coverage/.tmp`) eliminated	✅ (sequential `&&`)
80% branch threshold gates both projects independently	✅ server passes; ❌ client starts at ~53% — hard gate fails CI as decided (Theme 1 + Theme 3)
Coverage artifact uploaded in CI	✅

The client coverage starting below 80% is the expected consequence of choosing "all at once" + "hard gate immediately". A follow-up issue to raise component test coverage to 80% is the natural next step.

## Implementation complete — PR #495 ### What was built **4 commits on `feat/issue-425-browser-coverage`:** - `1a28e311` `build(deps): add @vitest/coverage-istanbul for browser-project coverage` - `bb374bf2` `feat(test): add Istanbul browser coverage via standalone client config` - `16f69fff` `feat(test): update test:coverage to run both server and client projects` - `eccecf35` `ci: add combined coverage gate to unit-tests job` ### Key implementation note Vitest 4 silently ignores `coverage` overrides placed inside individual `test.projects` entries — the global coverage block always wins. The fix is a standalone `vitest.client-coverage.config.ts` where the Istanbul coverage block sits at the root `test` level and is actually honoured. `vite.config.ts` retains the v8 block for the server project (`coverage/server`); the standalone config writes to `coverage/client`. ### Acceptance criteria status | Criterion | Status | |-----------|--------| | `npm run test:coverage` produces `coverage/server/lcov.info` (v8) | ✅ | | `npm run test:coverage` produces `coverage/client/lcov.info` (Istanbul) | ✅ | | `coverage/client/lcov.info` covers `.svelte` components | ✅ | | Race condition (`ENOTEMPTY` on `coverage/.tmp`) eliminated | ✅ (sequential `&&`) | | 80% branch threshold gates both projects independently | ✅ server passes; ❌ client starts at ~53% — hard gate fails CI as decided (Theme 1 + Theme 3) | | Coverage artifact uploaded in CI | ✅ | The client coverage starting below 80% is the expected consequence of choosing "all at once" + "hard gate immediately". A follow-up issue to raise component test coverage to 80% is the natural next step.

marcel referenced this issue

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

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

marcel closed this issue

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#425

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

Problem

Goal

Investigation paths

1. Fix the .tmp race condition

2. Enable browser coverage via Istanbul

3. Run projects sequentially

4. Use a single merged threshold

Acceptance criteria

Related

🏗️ Markus Keller — Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt — Fullstack Developer

Observations

Recommendations

Open Decisions

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

📋 Elicit — Requirements Engineer

Observations

Recommendations

Open Decisions

🔒 Nora Steiner (NullX) — Security Engineer

Observations

Recommendations

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

Recommendations

Open Decisions

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Observations

Recommendations

Decision Queue — Cross-Persona Open Items

Theme 1: Client coverage include scope and initial threshold

Theme 2: Combined vs. independent coverage thresholds

Theme 3: CI enforcement timing for the client gate

Theme 4: Script naming strategy

Theme 5: Coverage report persistence / hosting

Implementation complete — PR #495

What was built

Key implementation note

Acceptance criteria status

1. Fix the `.tmp` race condition

Theme 1: Client coverage `include` scope and initial threshold