Skip to content

Test _send's answer handling: silent 201, other 2xx, unreadable error body - #15

Merged
dmccoystephenson merged 1 commit into
mainfrom
trace/test-send-answer-handling
Oct 16, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
trace/test-send-answer-handling

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Stage B (unit-test expansion) cycle, scoped to TraceClient._send's handling of the server's answer. The README's wire-format section promises "A 201 is success; anything else is logged at DEBUG and dropped". Until now only the HTTPError path (a 401) was tested, which left three branches without a test:

  • test_a_201_is_delivered_without_logging_anything: success is silent on the trace logger.
  • test_a_success_status_other_than_201_is_logged_as_not_delivered: a 200 or a 204 goes through the non-error urlopen path and reaches the status != 201 log line.
  • test_an_error_answer_whose_body_cannot_be_read_still_logs_its_status: an HTTPError whose read() raises is swallowed, its status (503) is still logged at DEBUG, and the sender thread does not die (threading.excepthook is never called).

These are characterization tests only. No production code changed, and all three tests pass against the current behavior.

Test plan

  • python3 -m unittest -v locally on Python 3.8.10: Ran 48 tests, OK
  • Non-vacuous check: with status != 201 temporarily changed to status >= 300 and the inner try around answered.read() removed, the two branch tests failed (FAILED (failures=2)). The source was then restored.
  • CI Build green on 3.8 / 3.10 / 3.12

Notes

  • Only standard-library imports were added (urllib.error, urllib.request), and only in the test module.
  • No Java-visible behavior changed, so trace-client-java needs no matching change.
  • No tracking issue: the gap was found during triage, and the backlog held no open issues, so none were skipped.

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

… body

The README promises "A 201 is success; anything else is logged at DEBUG
and dropped", but only the HTTPError path (401) was exercised. Lock in that
a 201 logs nothing, that a 200/204 is logged as not delivered, and that an
error answer whose body read fails still logs its status without killing
the sender thread.

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

Copy link
Copy Markdown
Member Author

Self-review rubric (Stage B, unit-test expansion; anchored on CI run 38532190743):

  • Scope: PASS. Only tests/test_trace_client.py is modified (+36 lines): three tests plus two stdlib imports.
  • Tests-new: PASS (not applicable). No new public names were added.
  • Tests-fix: not applicable (characterization only, no fix). As a substitute for a non-vacuity check, a temporary mutation of _send (status != 201 changed to status >= 300, and the inner try around answered.read() removed) made the two branch tests fail (FAILED (failures=2)). The source was restored before commit.
  • Sibling structure: PASS. The tests sit next to test_report_does_not_raise_when_the_server_rejects_the_key and reuse _Capture.reply_status, the self.log capture, and the threading.excepthook patch idiom from test_the_sender_thread_survives_an_unexpected_error_from_send.
  • Sibling renames: not applicable. Nothing was renamed.
  • Docs: PASS. The behavior is unchanged, and README's "A 201 is success; anything else is logged at DEBUG and dropped" is now asserted directly.
  • Issue resolution: not applicable. No Closes, because the gap was found during triage.
  • CI: PASS. test (3.8), test (3.10) and test (3.12) are all green on head 43b7d67.
  • Stdlib-only: PASS. The only added imports are urllib.error and urllib.request.
  • Py3.8 floor: PASS. test (3.8) is green, and the local run was on 3.8.10.
  • Never-raises / never-blocks: not applicable. Production code is untouched.
  • Log level: PASS. No _LOG. lines were added, and every test asserts that all captured records are at DEBUG.
  • Version agreement: PASS. Version strings are untouched.
  • Exports: not applicable.
  • Java parity noted: PASS. The PR body says no Java-visible behavior changed.

Judgment call flagged for a human reviewer:

  • tests/test_trace_client.py:133 — test_a_201_is_delivered_without_logging_anything asserts that the trace log is completely empty on success. This deliberately characterizes the current behavior ("otherwise not at all"). If a success-path DEBUG line is ever wanted, this assertion is the one that will need to change.

Summary: a tests-only PR with green CI on the whole matrix. It is ready for human review and merge.

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


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit f7d736f into main Oct 16, 2026
3 checks 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