Skip to content

Commit 67658f4

Browse files
timvisher-ddclaude
timvisher-dd
and
claude
committed
Strengthen table tests with overlay structure and mid-stream assertions
Add two new tests: - agent-shell--table-overlay-structure-test: verifies that after all chunks arrive and markdown-overlays fires, there are exactly 12 overlays (header + separator + 10 data rows), each with correct before-string content and no spurious newlines. - agent-shell--table-mid-stream-overlay-cleanup-test: simulates the debounce timer firing mid-stream on a partial table, then verifies that the final overlay state after all chunks is correct (old partial overlays cleaned up). Also refactors shared test helpers and the chunk list into reusable functions/constants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 721f4e6 commit 67658f4

2 files changed

Lines changed: 175 additions & 169 deletions

File tree

‎tests/agent-shell-table-tests.el‎

Lines changed: 175 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,85 @@ where `shell-maker-define-major-mode' evals keymap structs as code."
3737
(use-local-map agent-shell-mode-map)))))
3838
buffer))
3939

40+
(defun agent-shell-table-test--fire-debounce ()
41+
"Fire pending markdown overlay debounce timer if present."
42+
(when (and (boundp 'agent-shell--markdown-overlay-timer)
43+
(timerp agent-shell--markdown-overlay-timer))
44+
(timer-event-handler agent-shell--markdown-overlay-timer)))
45+
46+
(defun agent-shell-table-test--table-overlays ()
47+
"Return table overlays in the current buffer, sorted by position.
48+
Each element is an alist with :start, :end, and :before-string."
49+
(let ((result nil))
50+
(dolist (ov (overlays-in (point-min) (point-max)))
51+
(when (eq (overlay-get ov 'invisible) 'markdown-overlays-tables)
52+
(push (list (cons :start (overlay-start ov))
53+
(cons :end (overlay-end ov))
54+
(cons :before-string
55+
(when-let ((bs (overlay-get ov 'before-string)))
56+
(substring-no-properties bs))))
57+
result)))
58+
(sort result (lambda (a b) (< (map-elt a :start) (map-elt b :start))))))
59+
60+
(defun agent-shell-table-test--send-tool-call (state tool-id)
61+
"Send a complete tool_call lifecycle (pending → meta → completed).
62+
STATE is agent-shell--state, TOOL-ID is the tool call identifier."
63+
(agent-shell--on-notification
64+
:state state
65+
:acp-notification `((method . "session/update")
66+
(params . ((update
67+
. ((toolCallId . ,tool-id)
68+
(sessionUpdate . "tool_call")
69+
(rawInput)
70+
(status . "pending")
71+
(title . "Bash")
72+
(kind . "execute")))))))
73+
(agent-shell--on-notification
74+
:state state
75+
:acp-notification `((method . "session/update")
76+
(params . ((update
77+
. ((_meta (claudeCode (toolResponse (stdout . "tool output")
78+
(stderr . "")
79+
(interrupted)
80+
(isImage)
81+
(noOutputExpected))
82+
(toolName . "Bash")))
83+
(toolCallId . ,tool-id)
84+
(sessionUpdate . "tool_call_update")))))))
85+
(agent-shell--on-notification
86+
:state state
87+
:acp-notification `((method . "session/update")
88+
(params . ((update
89+
. ((toolCallId . ,tool-id)
90+
(sessionUpdate . "tool_call_update")
91+
(status . "completed"))))))))
92+
93+
(defun agent-shell-table-test--send-message-chunks (state tokens)
94+
"Send agent_message_chunk notifications for each token in TOKENS.
95+
STATE is agent-shell--state."
96+
(dolist (token tokens)
97+
(agent-shell--on-notification
98+
:state state
99+
:acp-notification `((method . "session/update")
100+
(params . ((update
101+
. ((sessionUpdate . "agent_message_chunk")
102+
(content (type . "text")
103+
(text . ,token))))))))))
104+
105+
;;; The real-world chunks from the debug session, split exactly as ACP
106+
;;; delivered them. The table has 4 columns and ~10 data rows.
107+
(defconst agent-shell-table-test--chunks
108+
(list
109+
"Here's the comparison:\n\n| Policy"
110+
" | lab-green-003 | prod-green-003 | lab-v6.4-003 |\n|---|---|---|---|\n| dd_pastebin | `removed"
111+
"` | `removed` | `removed` |\n| onepassword_scim | `removed` | `removed` | `removed` |\n| us1_prod_dog_incidents_app | `removed` | `removed` | `removed` |"
112+
"\n| us1_prod_dog_pagerbeauty | `removed` | `removed` | `removed` |\n| us1_prod_dog_support_eng_access | `removed` | `removed` | `removed` |\n| us1-"
113+
"staging-fed-ssh | `removed` | `removed` | `removed` |\n| us1-staging-fed-dns | `removed` | `removed` | `removed` |\n| pci | `removed` | `removed` | `removed` |\n|"
114+
" production_ga | `removed` | `removed` | `removed` |\n| production_common_services | `removed` | `removed` | `removed` |"
115+
"\n\nAll 18 policies are aligned.")
116+
"Chunk sequence from a real debug session containing a markdown table.")
117+
118+
40119
(ert-deftest agent-shell--table-rows-not-split-across-lines-test ()
41120
"Markdown table rows must render with pipe-delimited cells on single lines.
42121
Regression test: table rows with backtick-wrapped content like `removed`
@@ -57,69 +136,20 @@ was streamed across multiple chunks with cell boundaries split mid-chunk."
57136
(cl-letf (((symbol-function 'agent-shell--make-diff-info)
58137
(cl-function (lambda (&key acp-tool-call) (ignore acp-tool-call)))))
59138
(with-current-buffer buffer
60-
;; Precede with a completed tool call (as in the real session).
61-
(agent-shell--on-notification
62-
:state agent-shell--state
63-
:acp-notification `((method . "session/update")
64-
(params . ((update
65-
. ((toolCallId . ,tool-id)
66-
(sessionUpdate . "tool_call")
67-
(rawInput)
68-
(status . "pending")
69-
(title . "Bash")
70-
(kind . "execute")))))))
71-
(agent-shell--on-notification
72-
:state agent-shell--state
73-
:acp-notification `((method . "session/update")
74-
(params . ((update
75-
. ((_meta (claudeCode (toolResponse (stdout . "tool output")
76-
(stderr . "")
77-
(interrupted)
78-
(isImage)
79-
(noOutputExpected))
80-
(toolName . "Bash")))
81-
(toolCallId . ,tool-id)
82-
(sessionUpdate . "tool_call_update")))))))
83-
(agent-shell--on-notification
84-
:state agent-shell--state
85-
:acp-notification `((method . "session/update")
86-
(params . ((update
87-
. ((toolCallId . ,tool-id)
88-
(sessionUpdate . "tool_call_update")
89-
(status . "completed")))))))
90-
;; Stream agent_message_chunk tokens containing a markdown
91-
;; table. These are the actual chunks from a real debug
92-
;; session, split exactly as the ACP delivered them.
93-
(dolist (token (list
94-
"Here's the comparison:\n\n| Policy"
95-
" | lab-green-003 | prod-green-003 | lab-v6.4-003 |\n|---|---|---|---|\n| dd_pastebin | `removed"
96-
"` | `removed` | `removed` |\n| onepassword_scim | `removed` | `removed` | `removed` |\n| us1_prod_dog_incidents_app | `removed` | `removed` | `removed` |"
97-
"\n| us1_prod_dog_pagerbeauty | `removed` | `removed` | `removed` |\n| us1_prod_dog_support_eng_access | `removed` | `removed` | `removed` |\n| us1-"
98-
"staging-fed-ssh | `removed` | `removed` | `removed` |\n| us1-staging-fed-dns | `removed` | `removed` | `removed` |\n| pci | `removed` | `removed` | `removed` |\n|"
99-
" production_ga | `removed` | `removed` | `removed` |\n| production_common_services | `removed` | `removed` | `removed` |"
100-
"\n\nAll 18 policies are aligned."))
101-
(agent-shell--on-notification
102-
:state agent-shell--state
103-
:acp-notification `((method . "session/update")
104-
(params . ((update
105-
. ((sessionUpdate . "agent_message_chunk")
106-
(content (type . "text")
107-
(text . ,token)))))))))
108-
;; Fire any pending debounce timers (streaming-dedup branch).
109-
(when (and (boundp 'agent-shell--markdown-overlay-timer)
110-
(timerp agent-shell--markdown-overlay-timer))
111-
(timer-event-handler agent-shell--markdown-overlay-timer))
112-
;; Verify: the table content is visible.
139+
(agent-shell-table-test--send-tool-call agent-shell--state tool-id)
140+
(agent-shell-table-test--send-message-chunks
141+
agent-shell--state agent-shell-table-test--chunks)
142+
(agent-shell-table-test--fire-debounce)
143+
;; Verify: the table content is visible in the raw text.
113144
(let ((visible-text (agent-shell-test--visible-buffer-string)))
114145
;; Header row must be intact on one line.
115146
(should (string-match-p
116147
"| Policy.*| lab-green-003.*| prod-green-003.*| lab-v6.4-003 |"
117148
visible-text))
118149
;; Separator row.
119150
(should (string-match-p "|---|---|---|---|" visible-text))
120-
;; A data row: policy name and all three `removed` cells
121-
;; must appear on the same logical line (no stray newlines
122-
;; splitting cell content from its row).
151+
;; Data rows: policy name and all three `removed` cells
152+
;; must appear on the same logical line.
123153
(should (string-match-p
124154
"| dd_pastebin.*|.*removed.*|.*removed.*|.*removed.*|"
125155
visible-text))
@@ -128,10 +158,8 @@ was streamed across multiple chunks with cell boundaries split mid-chunk."
128158
visible-text))
129159
;; Post-table text must be visible.
130160
(should (string-match-p "All 18 policies are aligned" visible-text)))
131-
;; Stronger check: no line in the visible text should consist
132-
;; of just "removed" (with optional backticks/whitespace).
133-
;; This is the specific regression symptom: cell content
134-
;; appearing on standalone lines outside the table structure.
161+
;; No line should consist of just "removed" — the regression
162+
;; symptom of cell content breaking out of the table.
135163
(let ((visible-text (agent-shell-test--visible-buffer-string)))
136164
(dolist (line (split-string visible-text "\n"))
137165
(should-not (string-match-p
@@ -140,5 +168,92 @@ was streamed across multiple chunks with cell boundaries split mid-chunk."
140168
(when (buffer-live-p buffer)
141169
(kill-buffer buffer)))))
142170

171+
(ert-deftest agent-shell--table-overlay-structure-test ()
172+
"Each table row must have exactly one overlay with correct before-string.
173+
After all chunks arrive and markdown overlays are applied, the overlay
174+
structure should show:
175+
- 1 header row overlay containing all column names
176+
- 1 separator overlay
177+
- N data row overlays, each containing the policy name and all cells"
178+
(let* ((buffer (agent-shell-table-test--setup-buffer))
179+
(agent-shell--state (agent-shell--make-state :buffer buffer))
180+
(agent-shell--transcript-file nil)
181+
(tool-id "toolu_overlay_test"))
182+
(map-put! agent-shell--state :client 'test-client)
183+
(map-put! agent-shell--state :request-count 1)
184+
(map-put! agent-shell--state :active-requests (list t))
185+
(unwind-protect
186+
(cl-letf (((symbol-function 'agent-shell--make-diff-info)
187+
(cl-function (lambda (&key acp-tool-call) (ignore acp-tool-call)))))
188+
(with-current-buffer buffer
189+
(agent-shell-table-test--send-tool-call agent-shell--state tool-id)
190+
(agent-shell-table-test--send-message-chunks
191+
agent-shell--state agent-shell-table-test--chunks)
192+
(agent-shell-table-test--fire-debounce)
193+
(let ((table-ovs (agent-shell-table-test--table-overlays)))
194+
;; 1 header + 1 separator + 10 data rows = 12 overlays
195+
(should (= 12 (length table-ovs)))
196+
;; First overlay is the header row.
197+
(let ((header-bs (map-elt (car table-ovs) :before-string)))
198+
(should (string-match-p "Policy" header-bs))
199+
(should (string-match-p "lab-green-003" header-bs))
200+
(should (string-match-p "prod-green-003" header-bs))
201+
(should (string-match-p "lab-v6.4-003" header-bs)))
202+
;; Each data row overlay (index 2+) must contain "removed"
203+
;; and the cell content must be on a single line.
204+
(dolist (ov (nthcdr 2 table-ovs))
205+
(let ((bs (map-elt ov :before-string)))
206+
(should (string-match-p "removed" bs))
207+
;; The before-string for a single-line row should
208+
;; NOT contain newlines (multi-line wrapping aside).
209+
;; If it does, cells are being split.
210+
(should-not (string-match-p "\n" bs)))))))
211+
(when (buffer-live-p buffer)
212+
(kill-buffer buffer)))))
213+
214+
(ert-deftest agent-shell--table-mid-stream-overlay-cleanup-test ()
215+
"Overlays from partial table rendering must be cleaned up after full table arrives.
216+
Simulates the debounce timer firing mid-stream (when only part of the
217+
table has been received), then checks that the final overlay state is
218+
correct after all chunks arrive."
219+
(let* ((buffer (agent-shell-table-test--setup-buffer))
220+
(agent-shell--state (agent-shell--make-state :buffer buffer))
221+
(agent-shell--transcript-file nil)
222+
(tool-id "toolu_midstream_test")
223+
(all-chunks agent-shell-table-test--chunks)
224+
;; Split: first 3 chunks = partial table, rest = completion.
225+
(early-chunks (seq-take all-chunks 3))
226+
(late-chunks (seq-drop all-chunks 3)))
227+
(map-put! agent-shell--state :client 'test-client)
228+
(map-put! agent-shell--state :request-count 1)
229+
(map-put! agent-shell--state :active-requests (list t))
230+
(unwind-protect
231+
(cl-letf (((symbol-function 'agent-shell--make-diff-info)
232+
(cl-function (lambda (&key acp-tool-call) (ignore acp-tool-call)))))
233+
(with-current-buffer buffer
234+
(agent-shell-table-test--send-tool-call agent-shell--state tool-id)
235+
;; Stream partial table
236+
(agent-shell-table-test--send-message-chunks
237+
agent-shell--state early-chunks)
238+
;; Fire debounce mid-stream (partial table gets overlaid)
239+
(agent-shell-table-test--fire-debounce)
240+
(let ((partial-ovs (agent-shell-table-test--table-overlays)))
241+
;; Partial table should have SOME overlays (header + sep + rows so far).
242+
(should (< 0 (length partial-ovs))))
243+
;; Stream remaining chunks
244+
(agent-shell-table-test--send-message-chunks
245+
agent-shell--state late-chunks)
246+
;; Fire debounce again (full table)
247+
(agent-shell-table-test--fire-debounce)
248+
(let ((final-ovs (agent-shell-table-test--table-overlays)))
249+
;; Full table: 1 header + 1 separator + 10 data rows = 12
250+
(should (= 12 (length final-ovs)))
251+
;; Every data row overlay should contain "removed".
252+
(dolist (ov (nthcdr 2 final-ovs))
253+
(should (string-match-p "removed"
254+
(map-elt ov :before-string)))))))
255+
(when (buffer-live-p buffer)
256+
(kill-buffer buffer)))))
257+
143258
(provide 'agent-shell-table-tests)
144259
;;; agent-shell-table-tests.el ends here

0 commit comments

Comments
 (0)