Skip to content

Test the telemetry client - #43

Merged
nrodd merged 1 commit into
tests/harness-and-cifrom
tests/telemetry-client
Sep 28, 2026
Merged

nrodd merged 1 commit into
tests/harness-and-cifrom
tests/telemetry-client

Conversation

@nrodd

@nrodd nrodd commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Chunk 1 of the test plan. One new file, src/telemetry.test.ts, 30 tests. No source changes.

Based on #42 (the harness). GitHub will retarget this to main when that merges. Independent of the other chunk PRs.

What it pins

Two properties matter more here than the payload shape, and each gets its own tests:

It never throws into the wizard. A rejected request is swallowed inside step(), and later events still send after an earlier one failed. A telemetry outage can't break an install.

It delivers nothing before authorize(). The endpoint rejects unauthenticated posts, so an event fired early is dropped, not queued — visible under --debug, invisible otherwise. Worth a test because "we'll flush it later" is the intuitive-but-wrong reading.

Beyond that:

  • Duration stamping — start gets duration_ms, complete gets total_duration_ms, neither is stamped on an intermediate step, and explicit metadata beats both defaults (an agent-reported duration has to win over the wizard's fallback).
  • Auth scheme — Bearer for an OAuth token, Basic for an API key, carried from login rather than hardcoded. Plus the User-Agent version and the abort signal.
  • finish() — reports on start when no start has gone out yet, on complete once one has. That's how a run that dies before the handoff still closes its funnel entry.
  • flush() — waits for in-flight requests, but gives up at its deadline when one never settles, so exit can't hang on a dead endpoint.
  • Disabled instance — sends nothing and prints nothing even under --debug.

One asymmetry, recorded on purpose

note() is gated on --debug alone, not on telemetry being enabled — so --no-telemetry --debug still prints breadcrumbs. They never leave the user's terminal and are never sent, so this looks intentional rather than a leak. There's now a test saying so, so a future reader doesn't have to guess.

Verification

30 tests passing, typecheck clean. I also mutation-checked two of the subtler assertions — flipping the metadata spread order and making finish() always send complete — and confirmed exactly those tests fail, then reverted.


Note

Low Risk
Test-only change with no runtime or production code modifications.

Overview
Adds src/telemetry.test.ts (~30 Vitest cases) for the wizard Telemetry client. No production code changes — this locks in delivery behavior via stubbed fetch, captured console.error debug output, and fake timers.

The suite emphasizes failure isolation (rejected requests never throw; later events still send) and pre-authorize() behavior (events are dropped, not queued; visible only under --debug). It also covers JSON POST shape, Bearer vs Basic auth, User-Agent, abort signals, duration stamping on start/complete, finish() routing, debug-only note() (including when telemetry is opted out), and flush() waiting vs deadline timeout.

Reviewed by Cursor Bugbot for commit 9c6cd46. Bugbot is set up for automated code reviews on this repo. Configure here.

Chunk 1. Adds src/telemetry.test.ts, 30 tests. No source changes.

Two properties matter more than the payload shape and both get their own
tests: the client never throws into the wizard (a rejected request is
swallowed and later events still send), and it delivers nothing before
authorize(), since the endpoint rejects unauthenticated posts and an
early event is dropped rather than queued.

Also pins the duration stamping (start gets duration_ms, complete gets
total_duration_ms, and explicit metadata beats both defaults), the
Bearer-vs-Basic scheme carried from login, finish()'s start-vs-complete
choice, and flush() giving up at its deadline so exit can't hang on an
endpoint that never answers.

One asymmetry worth recording: note() is gated on --debug alone, not on
telemetry being enabled. It never leaves the terminal, so that's fine,
but there's now a test saying so on purpose.
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