Skip to content

Test the constructor's error messages and its daemon sender thread - #20

Merged
dmccoystephenson merged 1 commit into
mainfrom
trace/test-constructor-errors-and-daemon-thread
Oct 20, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
trace/test-constructor-errors-and-daemon-thread

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Stage B (unit-test expansion) of the constructor. Only tests change, and they record what the code does today; no production code is touched.

  • test_constructor_errors_name_the_argument_they_reject: each ValueError (the client's only raising path) names the argument it rejects (base_url, application, version, install_id). The overlong-value errors also name MAX_TAG_LENGTH. Before this, the tests only checked that ValueError was raised.
  • test_the_sender_is_one_daemon_thread_named_after_the_application: an enabled client starts exactly one thread. It is a daemon, so it never keeps the host program from exiting, and it is named trace-client/<trimmed application>. close() stops it. Nothing tested the "daemon thread" part of the README's "Returns immediately" promise before this.
  • test_a_disabled_client_starts_no_thread: enabled=False, no key and TraceClient.disabled() start no thread. This backs the "does nothing and costs nothing" claim.

The thread checks only count threads that appear while the test runs, so threads left over from earlier tests that exit during it cannot make it fail.

Test plan

  • python3 -m unittest -v locally on Python 3.8.10: Ran 59 tests, OK (56 before)
  • CI Build jobs test (3.8), test (3.10), test (3.12) green

Deferred

  • The version tag is added even when it pushes an event past MAX_TAGS #16 (the version tag can push an event past MAX_TAGS) was not picked up. It needs an owner decision on whether the README promise that tags "always holds at least version" should hold at the tag cap. The change would also have to be made in step with trace-client-java, so it cannot land from this repo alone.

trace-client-java

Nothing a Java user can observe has changed (tests only), so no matching change is needed.

No tracking issue: the gaps were 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 CI and the diff, assuming FAIL unless shown otherwise):

  • Scope: PASS. Only tests/test_trace_client.py changes (+41 lines). No production code, formatting or renames.
  • Tests-new: PASS (not applicable). No new public names. The three new tests cover existing constructor behavior.
  • Tests-fix: not applicable (no bug fix). As a sensitivity check, daemon=True was temporarily changed to False and the base_url message to "a URL is required". Both new tests then failed with their stated messages. With trace_client.py restored they pass.
  • Sibling structure: PASS. The tests sit in TraceClientTest next to the other constructor tests and use the existing naming style, assertion messages, and the close-every-enabled-client convention.
  • Sibling renames: not applicable.
  • Docs: PASS. The README's "Returns immediately" row ("one daemon thread the client owns") and "Turning it off" ("does nothing and costs nothing") already describe what is now tested. No doc change is needed.
  • Issue resolution: not applicable. There is no Closes. The version tag is added even when it pushes an event past MAX_TAGS #16 is deferred with its reason recorded on the issue and in the PR body.
  • CI: PASS. test (3.8), test (3.10) and test (3.12) are all green on the head commit.
  • Stdlib-only: PASS. No new imports (threading was already imported).
  • Py3.8 floor: PASS. test (3.8) is green, and the local run was on 3.8.10 (Ran 59 tests, OK).
  • Never-raises / never-blocks, log level, version agreement, exports: not applicable. trace_client/ is unchanged.
  • Java parity noted: PASS. The PR body says nothing a Java user can observe has changed.

Judgment call: the thread tests take a snapshot of threading.enumerate() before construction and only count threads that appear after it. Leftover handler threads from earlier tests that exit during the test therefore cannot cause a false failure. A thread started concurrently by another test is not possible, because unittest runs tests one after another. No issues remain open.

tests/test_trace_client.py:400: the check uses startswith(argument + " "), which pins the current " is ..." wording. A future rewording that still names the argument somewhere else in the message would fail this test. This was chosen on purpose, because it stops version from matching inside install_id-style names.

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 1419e6b into main Oct 20, 2026
3 checks passed
@dmccoystephenson
dmccoystephenson deleted the trace/test-constructor-errors-and-daemon-thread branch October 20, 2026 09:44
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