Skip to content

Cover the write fallbacks, a window left open and broken quotes - #41

Merged
dmccoystephenson merged 1 commit into
mainfrom
feature/cover-undriven-branches
Nov 6, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
feature/cover-undriven-branches

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Stage B (unit-test expansion) cycle: seven characterization tests for TraceClient branches that no existing test drove. No production code is changed. The gaps were found with a JaCoCo branch report, run from the command line with no change to pom.xml.

  • installId_aConfigThatCannotBeAppendedToFallsBackToAnInMemoryIdAndIsLoggedFine and installIdFile_aWriteThatFailsYieldsAnInMemoryIdAndIsLoggedFine: the two "could not write" fallbacks promised by the README and Javadoc. Neither was covered before. The existing installId_unwritableConfig... and installIdFile_unwritable... tests put the path under a regular file, so the read fails first and the write is never attempted. The new tests fail at the write without relying on permission bits, so they hold when the suite runs as root.
  • coalesce_aWindowThatClosesLeavesOneOpenedLaterToItsOwnDeadline: a flush that closes one window leaves a later window open until its own deadline, then the timer is re-armed for it.
  • serverWideTags_dropKeysAndValuesWhoseQuoteIsNeverClosed: a broken quote on a key, and a double-quoted value ending in a backslash, are dropped without throwing.
  • serverWideTags_aSingleQuotedValueTakesACommentButNothingElseAfterIt: the single-quote counterpart of the existing double-quote cases.
  • serverWideConfig_aByteThatIsNotUtf8LeavesReportingOnYet and serverWideConfig_aByteOrderMarkHidesAFirstLineEnabledFalseYet: these pin an apparent bug, filed as An enabled: false in a server-wide config that is not plain UTF-8 is ignored #40. In both cases enabled: false is ignored and reporting stays on. The behaviour is left unchanged for the maintainer to decide, and the tests assert it as it stands.

Branch coverage of TraceClient.java went from 333 of 344 to 339 of 344. The five branches still missed cannot be driven deterministically:

  • TraceClient.java:377: the read-back after appending a server-id: finds none, which needs the file to change between the append and the read.
  • TraceClient.java:432: a path with no parent, which fails at the read first.
  • TraceClient.java:562: the end of a line reached while counting indent, which is unreachable because blank lines are skipped above it.
  • TraceClient.java:689: an empty value after a comment is stripped, which is unreachable because the text is trimmed and does not start with #.
  • TraceClient.java:881: a flush that fires after close(), which needs a race with the timer being cancelled.

Triage notes (skipped issues)

The open issues #13, #15, #27, #31 and #39 each ask for a maintainer decision on the wire format or the opt-out contract, and each lists options. None was picked up for autonomous implementation this cycle. #40, filed during this cycle, is in the same position.

Test plan

  • mvn -B verify run locally on Java 21: Tests run: 106, Failures: 0, Errors: 0, Skipped: 0 (99 before this change)
  • Each new test confirmed to fail: nine temporary mutations of TraceClient.java, one or more per test, each turned the targeted test red, and the file was restored afterwards
  • CI matrix green on Java 8, 17 and 21

Two tests use assumeTrue, so they are skipped rather than failed off Linux or POSIX. CI runs on ubuntu-latest, where both run.

No tracking issue: this gap was found during triage.

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code


drafted by Claude on behalf of Daniel Stephenson

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (scored against the diff and CI run 42155011165):

  • Scope: PASS. Only src/test/java/software/stephenson/trace/TraceClientTest.java is modified (+161 lines, 7 new tests, 2 new imports). There is no formatting churn.
  • Tests-new: PASS (not applicable). No public members were added.
  • Tests-fix: not applicable. No production change. In place of a stash-and-run, nine temporary mutations of TraceClient.java were applied one at a time, and each turned its targeted test red. The file was restored afterwards.
  • Sibling structure: PASS. The names follow the serverWideConfig_*, serverWideTags_*, installId_*, installIdFile_* and coalesce_* series, with Arrange/Act/Assert comments, the loopback HttpServer fixture, @TempDir and RecordingHandler.
  • Sibling renames: not applicable.
  • Docs: PASS. There is no behaviour change, so no Phase 7 row is affected. The behaviour in An enabled: false in a server-wide config that is not plain UTF-8 is ignored #40 is left for the maintainer, and the README line "An IO failure is logged at FINE and counts as enabled" remains true of it.
  • Issue resolution: not applicable (no Closes).
  • CI: PASS. build (8), build (17) and build (21) are green on head 8a1b8ab. Locally, mvn -B verify (Java 21) ran 106 tests with 0 failures (99 before).
  • No dependencies / single file / Java 8: PASS. pom.xml and src/main are untouched. java.nio.file.Paths and Files.createSymbolicLink are Java 7 APIs, and the Java 8 leg confirms this.
  • Never throws / non-blocking / environment seam / version agreement / opt-out contract: not applicable (no production change).

Observations:

  • TraceClientTest.java installId_aConfigThatCannotBeAppendedTo...: the setup depends on /proc/version reading fine and refusing a write. It is guarded by assumeTrue, so it is skipped rather than failed off Linux. The build (8) job log shows Tests run: 106, Failures: 0, Errors: 0, Skipped: 0, so it ran on CI, as a non-root user, as well as locally as root.
  • TraceClientTest.java coalesce_aWindowThatClosesLeavesOneOpenedLaterToItsOwnDeadline: the "late window has not closed yet" check allows about 800 ms of runner stall (2 s window, 1 s offset, 200 ms settle). A longer stall would fail it. This is a real but small flake risk, in line with the existing 200 ms check against a 1 s window in coalesce_sendsTheFirstAtOnce....
  • TraceClientTest.java installId_unwritableConfigFallsBackToAnInMemoryIdWithoutThrowing and installIdFile_unwritableYieldsAnInMemoryIdWithoutThrowing: both names say "unwritable", but both fail at the read. They are left unrenamed to keep this PR to additions, and each new test's comment says what the test above it actually drives.

Summary: the PR adds tests only, CI is green on all three JDKs, and no blocking findings remain.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit 674ccda into main Nov 6, 2026
3 checks passed
@dmccoystephenson
dmccoystephenson deleted the feature/cover-undriven-branches branch November 6, 2026 03:13
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