Skip to content

chore(sonar): resolve maintainability findings (74 → 1) - #251

Merged
ttbombadil merged 3 commits into
mainfrom
chore/sonar-maintainability-cleanup
Oct 13, 2026
Merged

ttbombadil merged 3 commits into
mainfrom
chore/sonar-maintainability-cleanup

Conversation

@ttbombadil

Copy link
Copy Markdown
Collaborator

Summary

Clears the Sonar maintainability findings that are worth fixing, without forcing the count to zero: 74 → 1. No Sonar rule is disabled and nothing is marked as a false positive. 73 of the 74 findings were in test code; one was in production code.

Rule Before After What was done
typescript:S5906 (expect(x.length).toBe(n)) 64 0 expect(x).toHaveLength(n) on exactly the reported lines; same semantics
typescript:S5976 (similar tests) 8 1 see below
typescript:S8980 (redundant act) 1 0 removed the act() around waitFor
typescript:S7776 (PWM_PINS as array) 1 0 one shared definition, see below

No new findings were introduced (Sonar re-analysis of this head: 74 → 1, 0 new issue keys; quality gate OK; coverage and duplication unchanged).

Commits

  1. test: use specific length assertions – 64 mechanical replacements in 25 test files.
  2. test: simplify maintainability findings – S8980 and the S5976 cases below.
  3. refactor: share canonical Uno PWM pins – the only production change.

Decisions worth reviewing

S5976 parametrized (5 groups). Only where each case stays a plain data row with its original title: font-scale-utils increase and decrease (6 cases each), arduino-output-parser fragment cases (4), and the logger sanitizer tests. The logger tests are two tables: input / marker / leaked plaintext (the secret must be replaced and must not be logged) and input / marker. The NOSONAR S2068 comments stay on the credential literals. I confirmed with a targeted mutation (changing the email marker) that exactly the email case fails.

serial-monitor.ui.test.tsx is intentionally left alone (the one remaining finding). The three tests share only their setup and assert different things (return value, line text, incomplete flag, ANSI stripping); a table would need union columns and read worse.

parser-output-pinmode.test.tsx was not parametrized; it was replaced. The two reported groups did not test production code: the file defined its own copy of extractPinModeData ("extracted from parser-output.tsx"), a local counting loop, and tests of a regex literal against strings. The behaviour they imitate is still live, though: IoRegistryRow derives pin modes from legacy usedAt operations ("pinMode:N") when pinModeModes is absent. The 16 tests on local copies are replaced by 10 tests that render ParserOutput with legacy records and assert the displayed mode, UNKNOWN for invalid/malformed operations, the conflict marker, that only pinMode operations count, and that pinModeModes takes precedence. Breaking the fallback regex in parser-output.tsx fails 8 tests; the file is restored.

S7776: not a local Array → Set. The UNO PWM pin list was defined three times (usePinPollingEngine.ts as an array, parser-output.tsx and shared/parsers/hardware-compatibility-parser.ts as sets) and written out a fourth time in the analogWrite warning text. UNO_PWM_PINS and isUnoPwmPin() now live in shared/utils/arduino-utils.ts (it imports types only, no cycles). Iteration order is unchanged and the warning text is derived from the same list and stays byte-identical (existing tests assert it). New tests cover every UNO pin number and the registry's ~ marker.

Verification

  • Targeted: all changed unit files (283 tests for the mechanical commit, then each parametrized/replaced file), the PWM consumers (use-pin-polling-engine, parser-output, static-io-parser-messages, code-parser).
  • Files in other Vitest projects: tests/core/sandbox-stress.test.ts (integration-toolchain, RUN_HEAVY_TESTS=1, the two tests with changed assertions), tests/server/services/serial-backpressure.test.ts (integration-docker, 4 tests), tests/server/load-suite.test.ts (load, the scalability test). All passed.
  • npm run check, npm run check:docs, ESLint over the whole tree, npm run build, and the full ./run-tests.sh (static analysis, knip, unit, toolchain integration, Docker, Playwright E2E, production build, security audit, Sonar gate with 0 violations); pre-push hook.

Left as is

  • typescript:S5976 in tests/client/serial-monitor.ui.test.tsx:363 (see above).
  • One multi-line expect(startHeartbeatLogs.length).toBe( in tests/server/telemetry-heartbeat-integration.test.ts has the same pattern but is not reported by Sonar; not touched.
  • Local note: npm run lint fails on this machine only because of git-ignored files in .tutor-quality-output/ (unrelated to the repository content); with that folder ignored, ESLint is clean.

Based on current main; independent of #250 (it touches no shared files).

🤖 Generated with Claude Code

ttbombadil and others added 3 commits October 12, 2026 19:16
Replace `expect(x.length).toBe(n)` with `expect(x).toHaveLength(n)` in the 64
places Sonar reports as typescript:S5906. Same semantics; a failing
assertion now reports the collection instead of a bare number.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Address the remaining Sonar test findings where the change is also an
improvement:

- font-scale-utils: the increase and decrease transitions are plain data
  tables; use it.each with the original titles (typescript:S5976).
- arduino-output-parser: the four uniform "ignore protocol fragment" cases
  become one it.each (typescript:S5976).
- logger sanitization: two tables, one with input / marker / leaked
  plaintext (secret must be replaced and not logged), one with input /
  marker; every case keeps its title (typescript:S5976).
- arduino-simulator-codechange: drop the redundant act() around waitFor,
  which already flushes its own updates (typescript:S8980).

parser-output-pinmode: Sonar's S5976 groups were not worth parametrizing.
The tests exercised a local copy of the legacy operations -> mode logic, a
local counting loop and a regex literal, none of which reaches production
code. The legacy fallback in IoRegistryRow (usedAt "pinMode:N" when
pinModeModes is absent) is still live, so replace them with tests that
render ParserOutput with legacy records and assert the displayed modes,
the conflict marker and the pinModeModes precedence. Confirmed they fail
when the fallback regex is broken.

serial-monitor.ui is intentionally left as is: its three tests share only
the setup and assert different things.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The set of PWM-capable Arduino UNO pins (3, 5, 6, 9, 10, 11) was defined
three times, as an array in the pin polling engine and as a Set in
parser-output and in the hardware-compatibility parser, and spelled out a
fourth time in the analogWrite warning text. Sonar flagged the array
(typescript:S7776); changing it locally would have kept the duplication.

Define UNO_PWM_PINS and isUnoPwmPin() once in shared/utils/arduino-utils.ts
(types-only imports, so no cycles) and use them in all three places. The
warning text is derived from the same list and stays identical. Iteration
order and behaviour are unchanged.

Tests: the shared list and predicate for every UNO pin number, and the
registry shows the "~" marker only for PWM-capable pins.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@ttbombadil
ttbombadil marked this pull request as ready for review October 12, 2026 17:36
@ttbombadil
ttbombadil merged commit db3fa4e into main Oct 13, 2026
6 checks passed
@ttbombadil
ttbombadil deleted the chore/sonar-maintainability-cleanup branch October 13, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant