Skip to content

Test that unsent reports are not logged and non-number values are omitted - #6

Merged
dmccoystephenson merged 1 commit into
mainfrom
feat/test-unsent-reports-not-logged
Oct 4, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
feat/test-unsent-reports-not-logged

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Stage B (unit-test expansion) cycle: report()'s input-guard branches. trace-client.ts is unchanged.

Summary

  • logs nothing for a report that was never going to be sent: README.md (the "Options and limits" section) promises that "A report that was never going to be sent is not logged: a disabled or closed client, or a blank or non-string name." The existing tests for those paths either passed no debug callback (sends nothing when disabled or without a key, ignores a blank name) or never exercised a non-string name. The new test covers a disabled client, a keyless client, blank names, the non-strings 42/undefined/null, and a report after close(), and asserts that both the server and the debug log are empty.
  • omits a value that is not a number: serialize() keeps value only when typeof value === "number" and it is finite. NaN and the infinities were already tested; a string, null and a boolean were not. The new test asserts that each is sent without value.

No tracking issue: this gap was found during triage, and the backlog had no open issues.

While writing these tests, report(name, null) was found to drop the event with a misleading could not serialize debug line. Per the test-expansion rule that production code is not changed in a characterization cycle, it was filed as #5 rather than fixed or pinned here.

Test plan

  • CI test job (npm ci, npm run typecheck, npm test on Node 22) is green on the PR head. Node is not installed in the authoring sandbox, so the tests were not run locally. CI is the only anchor.

Cross-client and changelog

  • No cross-client-visible behavior changed (tests only), so trace-client-python and trace-client-java need no change.
  • No CHANGELOG.md entry, because nothing a consumer vendors changed.

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


drafted by Claude on behalf of Daniel Stephenson

🤖 Generated with Claude Code

…tted

The README promises that a report which was never going to be sent -- a
disabled or closed client, or a blank or non-string name -- is not passed
to debug, but no test supplied a debug callback on those paths, and a
non-string name was never exercised. Nor was the rule that a value which
is not a number is left out of the body. Two characterization tests now
pin both; trace-client.ts is unchanged.

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

dmccoystephenson commented Oct 3, 2026 •

Copy link
Copy Markdown
Member Author

Self-review rubric (anchored on CI run 37107251970 at head c843498, plus the diff):

  • Scope: PASS. git diff --name-only origin/main...HEAD lists only tests/trace-client.test.ts (+29/-0).
  • Tests-new: PASS (not applicable). No public member was added; the PR adds two tests and no production code.
  • Tests-fix: PASS (not applicable). No bug fix is included. The report(name, null) defect found while writing the tests was filed as report(name, null) drops the event with a misleading 'could not serialize' debug line #5 and was neither fixed nor pinned here.
  • Sibling structure: PASS. Both tests sit in the TraceClient describe block next to ignores a blank name, follow its it("<behavior>") naming, use the loopback Capture server and the shared debug/log pair, and close every enabled client.
  • Sibling renames: PASS (not applicable). Nothing was renamed.
  • Docs: PASS. The test asserts the README claim (its "Options and limits" section) word for word: a disabled or closed client, or a blank or non-string name, is not logged. No doc needed changing.
  • Issue resolution: PASS (not applicable). There are no Closes references; the PR body records the gap as found during triage.
  • CI: PASS. The test job is green on the exact head SHA. npm test reported # tests 46, # pass 46, # fail 0 (44 tests before this PR, plus 2).
  • Zero-dependency / Edge gate / never-throws / no console / export surface / version agreement: PASS. trace-client.ts, tsconfig.json, package.json and the exported-values assertion are untouched, and the test adds no imports.
  • Changelog: PASS. Nothing consumers vendor changed, so no entry was added.
  • Cross-client parity noted: PASS. The PR body states that no cross-client-visible behavior changed.

Judgment calls:

  • tests/trace-client.test.ts:265 — The "logs nothing" test was only run against the current code and never against a mutation, because Node is absent from the authoring sandbox. By reading the code: if the guard at trace-client.ts:234 logged, or if a non-string name reached serialize(), the test's assert.deepEqual(log, []) would fail. Its weakness is that it also passes if an enabled client were silently broken. posts the event to the metrics endpoint… and similar tests already cover that, so it is left as is.
  • tests/trace-client.test.ts:265 — The test waits with sleep(100) before asserting that nothing arrived. This matches report() after close() is a no-op…. Every call there resolves synchronously without a request, so there is nothing for the sleep to race.

Summary: a test-only, low-risk PR, ready for human review.

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 050861b into main Oct 4, 2026
1 check passed
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