Skip to content

test(client-sdk/python): drop redundant exclude_none=True from heartbeat tracker tests (#73 review)#79

Merged
renerocksai merged 1 commit into
mainfrom
fix/python-heartbeat-tests-drop-redundant-exclude-none-73
May 4, 2026
Merged

test(client-sdk/python): drop redundant exclude_none=True from heartbeat tracker tests (#73 review)#79
renerocksai merged 1 commit into
mainfrom
fix/python-heartbeat-tests-drop-redundant-exclude-none-73

Conversation

@renerocksai
Copy link
Copy Markdown
Contributor

Refs #73. Follows up on the reviewer-bot's last comment on #76.

Context

a724ddb (currently on main) fixed the "session": null re-encode regression by adding @model_serializer(mode="wrap") to HeartbeatPayload, which strips the session key at the model layer whenever its value is None. The accompanying test_encoder_omits_none_session locks that contract in.

The reviewer-bot's last comment on #76 noted that with the serializer in place, the per-call-site model_dump_json(exclude_none=True) calls in tests/test_heartbeat.py are now redundant but harmless — they were originally added as a defensive workaround in 9bd6e4b before the serializer landed.

Quoting the relevant excerpt:

The exclude_none=True in the tracker tests is now redundant but harmless.

What this PR does

Drops exclude_none=True from the five nc.publish(...) call sites in client-sdk/python/tests/test_heartbeat.py (lines 142, 186, 227, 248, 269). The @model_serializer carries the contract on its own, so the test code is now consistent with how production-side build_heartbeat_payload callers already serialize (no exclude_none argument), and no longer signals that test authors needed a per-call workaround.

test_encoder_omits_none_session continues to assert that re-encoding a session=None payload omits the session key — that's the regression guard for the serializer, and it still passes.

Procedural note

The 9bd6e4b and a724ddb commits this PR builds on were unfortunately pushed straight to main by an earlier Claude Code session by mistake. This PR is the cleanup item flagged on top of that work, going through normal review for once.

Test plan

  • uv run pytest tests/test_heartbeat.py -v in client-sdk/python/ — 9 passed (including test_encoder_omits_none_session, which proves the serializer alone is sufficient)
  • uv run pytest (full suite) — 212 passed, including test_interop_e2e.py against the TS reference agent
  • uv run ruff check --no-cache . clean
  • uv run ruff format --check . clean
  • uv run mypy --no-incremental src tests examples clean

🤖 Generated with Claude Code

…tbeat tracker tests (#73 review)

The reviewer bot's last comment on PR #76 noted that `model_dump_json(exclude_none=True)`
at the five `nc.publish(...)` call sites in `test_heartbeat.py` is now redundant: the
`@model_serializer(mode="wrap")` added to `HeartbeatPayload` in commit a724ddb already
strips `session` when `None` at the model layer, so the per-call-site flag is dead
weight. Removing it makes the test code consistent with how production-side
`build_heartbeat_payload` callers already serialize (no `exclude_none` argument), and
removes the misleading signal that test authors needed a workaround.

`test_encoder_omits_none_session` (added in a724ddb) continues to lock in the
omission contract at the model layer, so this purely cosmetic cleanup cannot
regress on-wire compliance.

Refs #73.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer Bot commented May 4, 2026

Claude finished @renerocksai's task in 1m 22s —— View job


Review

  • Gather context (PR diff, source files)
  • Review changes for correctness and soundness
  • Post final review

The change is correct and the rationale holds up under inspection.

What I verified:

All 5 modified call sites construct HeartbeatPayload with explicit non-None session values ("shared", "stale", "listener"). This means exclude_none=True was already a no-op at each of these sites — the serialized output was identical with or without it. The argument was carrying no weight even before the @model_serializer landed.

The _drop_none_session serializer in heartbeat.py:72–77 correctly handles the only case that matters — session=None — and test_encoder_omits_none_session asserts that contract directly, both for model_dump_json() and model_dump(). That test still passes after this PR, which is the right regression guard.

No issues. Ready to merge.

@renerocksai renerocksai merged commit 5b3a1bc into main May 4, 2026
8 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