bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

SonarQube analyses the code but imports no test coverage, so its quality gate is measuring nothing #61

New Issue
Closed
opened 2026-08-19 11:05:02 -05:00 by bermudalamb · 2 comments
bermudalamb commented 2026-08-19 11:05:02 -05:00
Owner
Copy Link
Copy Source

Found during a Microsoft/React/SonarQube best-practices review.

Problem

.gitea/workflows/sonarqube.yml runs the scanner with four arguments:

-Dsonar.projectKey=redefined-designs
-Dsonar.login=${{ secrets.SONAR_TOKEN }}
-Dsonar.sources=backend/src,frontend/src
-Dsonar.exclusions=**/node_modules/**,**/dist/**

Three things are missing or wrong:

  1. No coverage is imported. There is no sonar.javascript.lcov.reportPaths, and neither test suite is run with coverage enabled before the scan. This project has 59 unit, 134 integration, and 83 end-to-end tests — and SonarQube reports 0% coverage for all of it. Any quality gate condition on coverage is either failing permanently or has been switched off, and in both cases it is telling nobody anything.
  2. No sonar.tests. Test files are not declared, so they are either analysed as production code or not at all. Test code should be scanned under the test rule set, not the main one.
  3. sonar.login is deprecated. SonarQube 10+ wants sonar.token. It is also redundant here — the token is already supplied via the SONAR_TOKEN environment variable, so it is passed twice, once on a command line where it is more likely to end up in a log.

There is also no sonar-project.properties; the whole configuration lives inline in the workflow, which means it cannot be run locally against the same settings CI uses.

Why it matters

Coverage that reads 0% is worse than no coverage metric, because it makes the quality gate look configured while measuring nothing. Untested new code cannot be distinguished from the well-tested existing code, so the "coverage on new code" gate — the single most useful thing SonarQube offers a project like this — is unavailable.

Suggested fix

  • Run the unit and integration suites with coverage in the Sonar workflow and point sonar.javascript.lcov.reportPaths at both lcov.info files.
  • Add sonar.tests for backend/tests and frontend/tests.
  • Replace sonar.login with sonar.token and drop the duplicate command-line copy.
  • Move the configuration into sonar-project.properties so it is reviewable and runnable locally.

Related

The project context already notes that SonarQube CI still uses the admin token rather than a dedicated gitea-ci user, flagged early on and never revisited. Worth handling in the same pass, since it is the same file.

Severity

Medium. Nothing is broken, but a quality gate that measures nothing provides false assurance.

Found during a Microsoft/React/SonarQube best-practices review. ## Problem `.gitea/workflows/sonarqube.yml` runs the scanner with four arguments: ``` -Dsonar.projectKey=redefined-designs -Dsonar.login=${{ secrets.SONAR_TOKEN }} -Dsonar.sources=backend/src,frontend/src -Dsonar.exclusions=**/node_modules/**,**/dist/** ``` Three things are missing or wrong: 1. **No coverage is imported.** There is no `sonar.javascript.lcov.reportPaths`, and neither test suite is run with coverage enabled before the scan. This project has 59 unit, 134 integration, and 83 end-to-end tests — and SonarQube reports **0% coverage** for all of it. Any quality gate condition on coverage is either failing permanently or has been switched off, and in both cases it is telling nobody anything. 2. **No `sonar.tests`.** Test files are not declared, so they are either analysed as production code or not at all. Test code should be scanned under the test rule set, not the main one. 3. **`sonar.login` is deprecated.** SonarQube 10+ wants `sonar.token`. It is also redundant here — the token is already supplied via the `SONAR_TOKEN` environment variable, so it is passed twice, once on a command line where it is more likely to end up in a log. There is also no `sonar-project.properties`; the whole configuration lives inline in the workflow, which means it cannot be run locally against the same settings CI uses. ## Why it matters Coverage that reads 0% is worse than no coverage metric, because it makes the quality gate look configured while measuring nothing. Untested new code cannot be distinguished from the well-tested existing code, so the "coverage on new code" gate — the single most useful thing SonarQube offers a project like this — is unavailable. ## Suggested fix - Run the unit and integration suites with coverage in the Sonar workflow and point `sonar.javascript.lcov.reportPaths` at both `lcov.info` files. - Add `sonar.tests` for `backend/tests` and `frontend/tests`. - Replace `sonar.login` with `sonar.token` and drop the duplicate command-line copy. - Move the configuration into `sonar-project.properties` so it is reviewable and runnable locally. ## Related The project context already notes that **SonarQube CI still uses the `admin` token rather than a dedicated `gitea-ci` user**, flagged early on and never revisited. Worth handling in the same pass, since it is the same file. ## Severity Medium. Nothing is broken, but a quality gate that measures nothing provides false assurance.
bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:37:45 -05:00
bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-20 09:00:24 -05:00
bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-20 09:00:28 -05:00
bermudalamb commented 2026-08-20 10:16:36 -05:00
Author
Owner
Copy Link
Copy Source

Implemented on feature/61-coverage-import. Coverage went from 0.0% to 69.6%, verified by a real scan rather than by reasoning about the config.

Two of the four asks were already done

#67 moved the scan configuration into sonar-project.properties and dropped the inline -D arguments, which covered the "runnable locally" and "deprecated sonar.login" items — the token now reaches the scanner only through SONAR_TOKEN, never on a command line. This change is the remaining two: import coverage, and declare sonar.tests.

Backend: both suites, not just the fast one

Line coverage
Unit suite alone 11%
Integration suite 73%

That gap is the whole argument for running both. Everything in src/routes is exercised only by the integration suite, so importing unit coverage alone would have reported the route files near zero — a number worse than none, because it would look measured. The two reports go to separate directories, since jest writes coverage/lcov.info by default and the second run would silently have overwritten the first.

That suite is workflow_dispatch-only after hanging for 3h12m post-run, so it runs here with --forceExit and the job carries a hard timeout-minutes. Jest confirmed during testing that it would otherwise have hung — the "Force exiting Jest" warning appeared on every run.

Frontend: instrumented dev server, collected by Playwright

The frontend has no unit tests, so coverage comes from vite-plugin-istanbul on the dev server with an auto-fixture flushing window.__coverage__ after each test, merged by nyc. 83 tests, 83 samples, 71% lines across all 34 source files. The 17 specs now import from a local fixtures.ts that re-exports @playwright/test, which is what lets the fixture attach without editing any test body.

Three things went wrong on the way, all caught by checking rather than assuming:

  • vite-plugin-istanbul@9 requires Vite ≥7 and this project is on Vite 5. Pinned to ^6 rather than forcing past the peer check with --legacy-peer-deps.
  • The plugin is ESM-only while vite.config.ts evaluates as CommonJS, so a static import failed the build outright. Loaded by dynamic import instead, which has the side benefit that a production build never even resolves the package.
  • My first include glob was src/*, which would have silently missed everything under src/admin, src/cart and src/customer — most of the app. Caught by curling the dev server for a nested module and checking for instrumentation, not by reading the config.

The production-safety check, both directions

Instrumentation is gated behind COVERAGE=true. A normal npm run build produces a bundle with zero __coverage__ occurrences; the dev server with the flag set instruments both nested and top-level modules. Both directions were checked, because a gate that is silently always-off looks identical to a gate that works.

This matters more than the coverage number: an instrumented bundle reaching customers would be larger, slower, and would publish the source structure through window.__coverage__.

Empty coverage fails loudly

npm run coverage:report refuses to write a report when .nyc_output holds no samples, and says which of the two likely causes it is. The guard was fired deliberately — deleting .nyc_output exits 1 with the explanation, restoring it exits 0.

This is here because the failure mode is silence, not error: if Playwright reuses an already-running uninstrumented dev server, every test passes, nothing is collected, and the resulting 0% reads as "the tests stopped covering things" rather than "collection was never switched on". That is the third time this shape has appeared — after SonarQube skipping the entire frontend while exiting EXECUTION SUCCESS (#67) and an ESLint matcher silently matching no files (#60).

Worth knowing when reading the number

End-to-end coverage flatters. Istanbul marks a line covered when the browser ran it, so a component rendered during a test counts as covered with nothing asserting anything about it. Backend coverage means considerably more per percentage point. The practical effect is that the 80% gate on new code is easier to clear on frontend changes than backend ones — which is backwards from where the risk actually sits.

Accepted deliberately: the alternative left every frontend pull request failing a gate it could never satisfy. Recorded in the design doc, the project context, and now #72 rather than left to be discovered.

Verification

  • Backend unit 78 pass, integration 134 pass, e2e 83 pass
  • Lint and build clean in both workspaces
  • Local scan imports all three lcov files: Analysing [backend/coverage/unit, backend/coverage/integration, frontend/coverage]
  • Project coverage 69.6%, line coverage 72.3%, 2205 lines to cover

Follow-ups filed

  • #71 — CI authenticates as admin rather than a restricted analysis account. This issue's "Related" note, split out so a permissions change is not buried in a CI-config commit. Documented in docs/ci/sonarqube-ci-identity.md.
  • #72 — keeping the pipeline honest: a frontend unit suite, guards against a partial coverage collapse, and root-causing the integration hang that --forceExit currently papers over. Contract documented in docs/ci/coverage-pipeline-contract.md.

One thing to expect: the SonarQube job is now much longer, since it runs all three suites plus a Postgres service before scanning. That cost is what makes the number real.

Not pushed — left local per the usual arrangement.

Implemented on `feature/61-coverage-import`. **Coverage went from 0.0% to 69.6%**, verified by a real scan rather than by reasoning about the config. ## Two of the four asks were already done #67 moved the scan configuration into `sonar-project.properties` and dropped the inline `-D` arguments, which covered the "runnable locally" and "deprecated `sonar.login`" items — the token now reaches the scanner only through `SONAR_TOKEN`, never on a command line. This change is the remaining two: import coverage, and declare `sonar.tests`. ## Backend: both suites, not just the fast one | | Line coverage | | --- | --- | | Unit suite alone | **11%** | | Integration suite | **73%** | That gap is the whole argument for running both. Everything in `src/routes` is exercised only by the integration suite, so importing unit coverage alone would have reported the route files near zero — a number worse than none, because it would look measured. The two reports go to separate directories, since jest writes `coverage/lcov.info` by default and the second run would silently have overwritten the first. That suite is `workflow_dispatch`-only after hanging for 3h12m post-run, so it runs here with `--forceExit` and the job carries a hard `timeout-minutes`. Jest confirmed during testing that it would otherwise have hung — the "Force exiting Jest" warning appeared on every run. ## Frontend: instrumented dev server, collected by Playwright The frontend has no unit tests, so coverage comes from `vite-plugin-istanbul` on the dev server with an auto-fixture flushing `window.__coverage__` after each test, merged by nyc. **83 tests, 83 samples, 71% lines across all 34 source files.** The 17 specs now import from a local `fixtures.ts` that re-exports `@playwright/test`, which is what lets the fixture attach without editing any test body. Three things went wrong on the way, all caught by checking rather than assuming: - `vite-plugin-istanbul@9` requires Vite ≥7 and this project is on Vite 5. Pinned to `^6` rather than forcing past the peer check with `--legacy-peer-deps`. - The plugin is **ESM-only** while `vite.config.ts` evaluates as CommonJS, so a static import failed the build outright. Loaded by dynamic import instead, which has the side benefit that a production build never even resolves the package. - My first include glob was `src/*`, which **would have silently missed everything under `src/admin`, `src/cart` and `src/customer`** — most of the app. Caught by curling the dev server for a nested module and checking for instrumentation, not by reading the config. ## The production-safety check, both directions Instrumentation is gated behind `COVERAGE=true`. A normal `npm run build` produces a bundle with zero `__coverage__` occurrences; the dev server with the flag set instruments both nested and top-level modules. Both directions were checked, because a gate that is silently always-off looks identical to a gate that works. This matters more than the coverage number: an instrumented bundle reaching customers would be larger, slower, and would publish the source structure through `window.__coverage__`. ## Empty coverage fails loudly `npm run coverage:report` refuses to write a report when `.nyc_output` holds no samples, and says which of the two likely causes it is. **The guard was fired deliberately** — deleting `.nyc_output` exits 1 with the explanation, restoring it exits 0. This is here because the failure mode is silence, not error: if Playwright reuses an already-running uninstrumented dev server, every test passes, nothing is collected, and the resulting 0% reads as "the tests stopped covering things" rather than "collection was never switched on". That is the third time this shape has appeared — after SonarQube skipping the entire frontend while exiting `EXECUTION SUCCESS` (#67) and an ESLint matcher silently matching no files (#60). ## Worth knowing when reading the number **End-to-end coverage flatters.** Istanbul marks a line covered when the browser ran it, so a component rendered during a test counts as covered with nothing asserting anything about it. Backend coverage means considerably more per percentage point. The practical effect is that the 80% gate on new code is easier to clear on frontend changes than backend ones — which is backwards from where the risk actually sits. Accepted deliberately: the alternative left every frontend pull request failing a gate it could never satisfy. Recorded in the design doc, the project context, and now #72 rather than left to be discovered. ## Verification - Backend unit 78 pass, integration 134 pass, e2e 83 pass - Lint and build clean in both workspaces - Local scan imports all three lcov files: `Analysing [backend/coverage/unit, backend/coverage/integration, frontend/coverage]` - Project coverage **69.6%**, line coverage 72.3%, 2205 lines to cover ## Follow-ups filed - **#71** — CI authenticates as `admin` rather than a restricted analysis account. This issue's "Related" note, split out so a permissions change is not buried in a CI-config commit. Documented in `docs/ci/sonarqube-ci-identity.md`. - **#72** — keeping the pipeline honest: a frontend unit suite, guards against a partial coverage collapse, and root-causing the integration hang that `--forceExit` currently papers over. Contract documented in `docs/ci/coverage-pipeline-contract.md`. One thing to expect: the SonarQube job is now much longer, since it runs all three suites plus a Postgres service before scanning. That cost is what makes the number real. Not pushed — left local per the usual arrangement.
bermudalamb added reference feature/61-coverage-import 2026-08-20 10:29:15 -05:00
bermudalamb commented 2026-08-20 15:27:58 -05:00
Author
Owner
Copy Link
Copy Source

Closing — implemented and merged 2026-08-20

Landed in 332c1e7 "feat(ci): import test coverage into SonarQube (#61)", authored 2026-08-20 10:13 CDT, merged to main the same day as PR #73 (0587c18) from feature/61-coverage-import, with 261d087 adding the pipeline contract docs.

Confirmed against the server rather than the config: SonarQube reports coverage 69.6%, line coverage 72.3%, across 2,205 lines to cover. The issue's title complaint — "imports no test coverage, so its quality gate is measuring nothing" — is no longer true. All three lcov reports are declared in sonar-project.properties and all three are read on every scan.

One clarification worth leaving on the record

A later issue quoted this one as having "addressed new_coverage". That overstates what this issue was for. This issue asked for coverage to be imported and measured, and it is. It did not promise the 80% gate threshold would be met — and it is not: new_coverage currently reads 71.2% against a threshold of 80, and is the only remaining failing gate condition.

That shortfall is #79, along with a structural problem found while measuring it: sonar.projectVersion was never set, so SonarQube had been treating the entire codebase as new code. That part is now fixed (PR #80).

So this issue is done on its own terms; the gate being red is a different issue's business.

Follow-ups from this work, still open

  • #71 — CI authenticates to SonarQube as admin rather than a restricted analysis account
  • #72 — keeping the pipeline honest: a frontend unit suite, guards against partial coverage collapse, and root-causing the integration-suite hang that --forceExit papers over
## Closing — implemented and merged 2026-08-20 Landed in `332c1e7` *"feat(ci): import test coverage into SonarQube (#61)"*, authored 2026-08-20 10:13 CDT, merged to `main` the same day as PR #73 (`0587c18`) from `feature/61-coverage-import`, with `261d087` adding the pipeline contract docs. Confirmed against the server rather than the config: SonarQube reports **coverage 69.6%**, line coverage 72.3%, across 2,205 lines to cover. The issue's title complaint — "imports no test coverage, so its quality gate is measuring nothing" — is no longer true. All three lcov reports are declared in `sonar-project.properties` and all three are read on every scan. ## One clarification worth leaving on the record A later issue quoted this one as having "addressed `new_coverage`". That overstates what this issue was for. This issue asked for coverage to be *imported and measured*, and it is. It did not promise the 80% gate threshold would be met — and it is not: `new_coverage` currently reads 71.2% against a threshold of 80, and is the only remaining failing gate condition. That shortfall is **#79**, along with a structural problem found while measuring it: `sonar.projectVersion` was never set, so SonarQube had been treating the entire codebase as new code. That part is now fixed (PR #80). So this issue is done on its own terms; the gate being red is a different issue's business. ## Follow-ups from this work, still open - **#71** — CI authenticates to SonarQube as `admin` rather than a restricted analysis account - **#72** — keeping the pipeline honest: a frontend unit suite, guards against partial coverage collapse, and root-causing the integration-suite hang that `--forceExit` papers over
bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-20 15:44:25 -05:00
bermudalamb moved this to Released in Code Quality and Hardening on 2026-08-21 12:12:07 -05:00
Sign in to join this conversation.
feature/61-coverage-import
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Code Quality and Hardening
Released
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#61