Repository navigation
Cover the write fallbacks, a window left open and broken quotes - #41
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Self-review rubric (scored against the diff and CI run 42155011165):
Observations:
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stage B (unit-test expansion) cycle: seven characterization tests for
TraceClientbranches 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 topom.xml.installId_aConfigThatCannotBeAppendedToFallsBackToAnInMemoryIdAndIsLoggedFineandinstallIdFile_aWriteThatFailsYieldsAnInMemoryIdAndIsLoggedFine: the two "could not write" fallbacks promised by the README and Javadoc. Neither was covered before. The existinginstallId_unwritableConfig...andinstallIdFile_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_aByteThatIsNotUtf8LeavesReportingOnYetandserverWideConfig_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 casesenabled: falseis 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.javawent 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 aserver-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 afterclose(), 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 verifyrun locally on Java 21:Tests run: 106, Failures: 0, Errors: 0, Skipped: 0(99 before this change)TraceClient.java, one or more per test, each turned the targeted test red, and the file was restored afterwardsTwo tests use
assumeTrue, so they are skipped rather than failed off Linux or POSIX. CI runs onubuntu-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