Skip to content

Commit ef7589e

Browse files
committed
test: close the gaps the self-review surfaced
Four targeted regression tests, each justified by what it would catch: 1. test_should_swallow_placeholder_clear_error_on_strict_adapter (test_thread_faithful.py) Closes the meaningful gap: the try/except wrap added in 8dd34d1 for adapters like Telegram that reject whitespace-only edit_message was untested. Mock adapter now raises ValueError when the placeholder-clear " " edit comes through; the test asserts (a) thread.post() returns a SentMessage instead of propagating, (b) exactly one clear attempt was made (no retry loop), and (c) the warn log fires with the exact expected message. Mutation-tested: narrowing the `except Exception` to `except RuntimeError` makes this test fail, confirming the swallow path is actually exercised. 2. test_should_rebind_adapter_when_data_is_already_a_channelimpl (test_channel_faithful.py) Symmetric with the ThreadImpl rebind regression in test_serialization.py. ChannelImpl.from_json had the same early-return-skips-rebinding bug fix in 1ce9cbf, but no test was added for the channel path. 3. test_strips_gchat_custom_link_with_balanced_parens_in_url (test_gchat_format_extended.py) Pins the plain-text-extraction behavior for URLs containing `)`. Worked already because the strip regex stops at `|`/`>`, but a future regex tweak could regress this without failing any other test. 4. test_gchat_multiple_custom_links_in_one_paragraph (test_gchat_format_extended.py) Asserts the exact (text, link, text, link, text, link, text) sequence for a paragraph with three `<url|text>` tokens. Catches off-by-one errors in the placeholder-substitution split logic that would otherwise pass single-link tests but produce wrong ordering or dropped fragments. 3450 tests pass. https://claude.ai/code/session_01XE1bMoQ5BjCvgi1iLGKKhG
1 parent d62e0e6 commit ef7589e

3 files changed

Lines changed: 123 additions & 0 deletions

File tree

tests/test_channel_faithful.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,30 @@ def test_should_sync_adapter_name_when_explicit_adapter_is_bound(self):
669669
assert channel.adapter.name == "teams"
670670
assert channel.to_json()["adapterName"] == "teams"
671671

672+
def test_should_rebind_adapter_when_data_is_already_a_channelimpl(self):
673+
"""Idempotent path: when ``data`` is already a ChannelImpl (e.g. revived
674+
via ``object_hook``), passing an explicit ``adapter=`` must still rebind
675+
it — an early-return shortcut would leave ``_adapter`` stale. Symmetric
676+
with the ThreadImpl regression in test_serialization.py."""
677+
from chat_sdk.testing import create_mock_adapter as _create
678+
679+
first = _create("slack")
680+
second = _create("teams")
681+
original = ChannelImpl.from_json(
682+
{
683+
"_type": "chat:Channel",
684+
"id": "C123",
685+
"adapter_name": "slack",
686+
"is_dm": False,
687+
},
688+
first,
689+
)
690+
rebound = ChannelImpl.from_json(original, second)
691+
692+
# Rebind applied even though data was already a ChannelImpl:
693+
assert rebound.adapter.name == "teams"
694+
assert rebound.to_json()["adapterName"] == "teams"
695+
672696

673697
# ===========================================================================
674698
# deriveChannelId (tested in channel.test.ts alongside ChannelImpl)

tests/test_gchat_format_extended.py

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,45 @@ def _walk(node: object) -> None:
127127
_walk(ast)
128128
assert link_urls == ["https://example.com"]
129129

130+
def test_gchat_multiple_custom_links_in_one_paragraph(self):
131+
"""A single paragraph with multiple `<url|text>` tokens must produce
132+
the right number of link nodes in the right order, with the
133+
surrounding text correctly split between them. Catches regressions
134+
in the placeholder-substitution split logic (off-by-one, dropped
135+
text, wrong link order)."""
136+
converter = _converter()
137+
ast = converter.to_ast(
138+
"Contact <mailto:a@example.com|Alice>, <mailto:b@example.com|Bob>, or <https://example.com|the site>."
139+
)
140+
141+
# Collect (text, link) sequence in document order.
142+
flat: list[tuple[str, dict | str]] = []
143+
144+
def _walk(node: object) -> None:
145+
if isinstance(node, dict):
146+
ntype = node.get("type")
147+
if ntype == "text":
148+
flat.append(("text", node.get("value", "")))
149+
elif ntype == "link":
150+
children = node.get("children", []) or []
151+
label = "".join(c.get("value", "") for c in children if isinstance(c, dict))
152+
flat.append(("link", {"url": node.get("url", ""), "text": label}))
153+
else:
154+
for child in node.get("children", []) or []:
155+
_walk(child)
156+
157+
_walk(ast)
158+
159+
assert flat == [
160+
("text", "Contact "),
161+
("link", {"url": "mailto:a@example.com", "text": "Alice"}),
162+
("text", ", "),
163+
("link", {"url": "mailto:b@example.com", "text": "Bob"}),
164+
("text", ", or "),
165+
("link", {"url": "https://example.com", "text": "the site"}),
166+
("text", "."),
167+
]
168+
130169
def test_gchat_custom_link_parses_url_with_balanced_parens(self):
131170
"""URLs containing `(...)` (e.g. Wikipedia-style) must round-trip
132171
intact. The Markdown parser doesn't implement CommonMark's balanced-
@@ -425,6 +464,17 @@ def test_strips_gchat_custom_link_to_label(self):
425464
== "See Example Site for details"
426465
)
427466

467+
def test_strips_gchat_custom_link_with_balanced_parens_in_url(self):
468+
"""The plain-text path must also strip cleanly when the URL contains
469+
unescaped `)`. Regex stops at `|`/`>`, not `)`, so the label is what
470+
survives — but pin it explicitly so a future regex tweak can't
471+
regress this without failing the test."""
472+
converter = _converter()
473+
assert (
474+
converter.extract_plain_text("See <https://en.wikipedia.org/wiki/Foo_(bar)|Wiki> for info")
475+
== "See Wiki for info"
476+
)
477+
428478

429479
# ---------------------------------------------------------------------------
430480
# render_postable

tests/test_thread_faithful.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,55 @@ async def test_should_clear_placeholder_when_stream_is_whitespace_only(self):
416416
assert isinstance(final_edit[2], PostableMarkdown)
417417
assert final_edit[2].markdown == " "
418418

419+
# Python-specific regression: when the placeholder-clear edit_message(" ")
420+
# raises (e.g. Telegram rejects whitespace-only content with a
421+
# ValidationError), `_fallback_stream` must log + swallow the error so
422+
# `thread.post()` still returns a SentMessage. The previous test pinned
423+
# the happy path; this one pins the defensive try/except added in
424+
# commit 8dd34d1 specifically for adapters that reject blank text.
425+
@pytest.mark.asyncio
426+
async def test_should_swallow_placeholder_clear_error_on_strict_adapter(self):
427+
adapter = create_mock_adapter()
428+
state = create_mock_state()
429+
logger = MockLogger()
430+
431+
# Telegram's adapter raises ValidationError when text.strip() is empty.
432+
# Simulate by intercepting edit_message and raising on the " " payload.
433+
clear_attempts: list[PostableMarkdown] = []
434+
original_edit = adapter.edit_message
435+
436+
async def strict_edit(thread_id: str, message_id: str, message: Any) -> RawMessage:
437+
if isinstance(message, PostableMarkdown) and not message.markdown.strip():
438+
clear_attempts.append(message)
439+
raise ValueError("Message text cannot be empty")
440+
return await original_edit(thread_id, message_id, message)
441+
442+
adapter.edit_message = strict_edit # type: ignore[assignment]
443+
444+
thread = _make_thread(adapter, state, logger=logger)
445+
# Whitespace-only stream triggers the placeholder-clear branch.
446+
text_stream = _create_text_stream([" ", "\n", " \n"])
447+
448+
# Must not raise — the SDK should log and fall through to the
449+
# upstream "leave placeholder visible" behavior on rejection.
450+
sent = await thread.post(text_stream)
451+
452+
# Placeholder was posted, then exactly one clear edit was attempted
453+
# (and rejected). No retry, no infinite loop.
454+
assert adapter._post_calls[0] == ("slack:C123:1234.5678", "...")
455+
assert len(clear_attempts) == 1
456+
assert clear_attempts[0].markdown == " "
457+
# The warn log fired with the expected message (asserting the exact
458+
# string so a refactor that changes the log can't silently break the
459+
# observability contract).
460+
assert any(
461+
call[0] == "fallbackStream placeholder-clear edit failed; placeholder will remain visible"
462+
for call in logger.warn.calls
463+
), [c[0] for c in logger.warn.calls]
464+
# Stream contract still holds — we got a SentMessage back.
465+
assert sent is not None
466+
assert sent.id == "msg-1"
467+
419468
# it("should handle empty stream with disabled placeholder")
420469
@pytest.mark.asyncio
421470
async def test_should_handle_empty_stream(self):

0 commit comments

Comments
 (0)