From 921598f30e470482efd3ce4981ee9893c8be60cf Mon Sep 17 00:00:00 2001
From: Craig Jennings <c@cjennings.net>
Date: Fri, 24 Jul 2026 10:58:25 -0500
Subject: fix(test): stop the syncing-p tests leaking in-flight state

- They cleared the module's state hash on entry, not on exit.
- One marks "proton" syncing to exercise the dispatcher's in-flight guard.
- A sibling dispatch test uses a calendar also named "proton".
- ERT runs them in that order, so the dispatch became a no-op and it failed.
- Now each test let-binds a private hash, matching the sibling test files.
- Added a test that pins the isolation itself.

The failure was invisible from both directions. make test runs each file in its
own Emacs, and the editor hook skipped this family for exceeding its file cap.
It only appears when the 63 calendar-sync files run in one process.
---
 tests/test-calendar-sync--syncing-p.el | 116 +++++++++++++++++++++------------
 tests/test-validate-el-hook.bats       |  86 +++++++++++++++---------
 2 files changed, 128 insertions(+), 74 deletions(-)

(limited to 'tests')

diff --git a/tests/test-calendar-sync--syncing-p.el b/tests/test-calendar-sync--syncing-p.el
index b346bf77..df8bcd52 100644
--- a/tests/test-calendar-sync--syncing-p.el
+++ b/tests/test-calendar-sync--syncing-p.el
@@ -4,81 +4,111 @@
 ;; Unit tests for `calendar-sync--syncing-p' (the per-calendar in-flight check
 ;; that lets the dispatcher skip an overlapping timer tick) and for the
 ;; load-state sanitize that clears a stale `syncing' status in a fresh process.
+;;
+;; Every test runs inside `test-cs-syncing--with-fresh-state', which let-binds
+;; a private state hash.  These tests previously cleared the module's global
+;; hash on entry and left whatever they wrote in it on exit, which leaked:
+;; `...-sync-calendar-skips-when-in-flight' marks "proton" as syncing to
+;; exercise the guard, and `test-calendar-sync--sync-dispatch-normal-ics-fetcher'
+;; in the sibling dispatch file dispatches a calendar also named "proton".
+;; ERT runs them in that order, so the leftover in-flight status made the
+;; dispatch a no-op and the sibling failed -- but only when the calendar-sync
+;; files ran in one process.  `make test' runs each file separately and the
+;; editor hook skipped this family for being over its file cap, so nothing
+;; caught it.  Let-binding is what the sibling files already do
+;; (test-calendar-sync.el, test-calendar-sync-async-worker.el); this file was
+;; the odd one out.
 
 ;;; Code:
 
 (require 'ert)
 (require 'calendar-sync)
 
-(defun test-cs-syncing--reset ()
-  "Clear the module's per-calendar state hash."
-  (clrhash calendar-sync--calendar-states))
+(defmacro test-cs-syncing--with-fresh-state (&rest body)
+  "Run BODY with a private, empty per-calendar state hash.
+Let-bound rather than cleared in place, so nothing this test writes can
+reach a later test."
+  (declare (indent 0))
+  `(let ((calendar-sync--calendar-states (make-hash-table :test 'equal)))
+     ,@body))
 
 ;;; calendar-sync--syncing-p
 
 (ert-deftest test-calendar-sync--syncing-p-normal-true-when-syncing ()
   "Normal: a calendar whose status is `syncing' reads as in-flight."
-  (test-cs-syncing--reset)
-  (calendar-sync--set-calendar-state "google" '(:status syncing))
-  (should (calendar-sync--syncing-p "google")))
+  (test-cs-syncing--with-fresh-state
+    (calendar-sync--set-calendar-state "google" '(:status syncing))
+    (should (calendar-sync--syncing-p "google"))))
 
 (ert-deftest test-calendar-sync--syncing-p-boundary-nil-when-no-state ()
   "Boundary: a calendar with no recorded state is not in-flight."
-  (test-cs-syncing--reset)
-  (should-not (calendar-sync--syncing-p "never-seen")))
+  (test-cs-syncing--with-fresh-state
+    (should-not (calendar-sync--syncing-p "never-seen"))))
 
 (ert-deftest test-calendar-sync--syncing-p-error-nil-for-terminal-status ()
   "Error: a terminal status (ok / error) is not in-flight."
-  (test-cs-syncing--reset)
-  (calendar-sync--set-calendar-state "google" '(:status ok))
-  (should-not (calendar-sync--syncing-p "google"))
-  (calendar-sync--set-calendar-state "proton" '(:status error))
-  (should-not (calendar-sync--syncing-p "proton")))
+  (test-cs-syncing--with-fresh-state
+    (calendar-sync--set-calendar-state "google" '(:status ok))
+    (should-not (calendar-sync--syncing-p "google"))
+    (calendar-sync--set-calendar-state "proton" '(:status error))
+    (should-not (calendar-sync--syncing-p "proton"))))
 
 ;;; Dispatcher guard: an in-flight calendar skips both leaf syncers
 
 (ert-deftest test-calendar-sync--sync-calendar-skips-when-in-flight ()
   "Normal: `calendar-sync--sync-calendar' does not launch a second sync for a
 calendar already marked syncing, so an overlapping timer tick is a no-op."
-  (test-cs-syncing--reset)
-  (let ((api-calls '()) (ics-calls '()))
-    (cl-letf (((symbol-function 'calendar-sync--sync-calendar-api)
-               (lambda (cal) (push cal api-calls)))
-              ((symbol-function 'calendar-sync--sync-calendar-ics)
-               (lambda (cal) (push cal ics-calls))))
-      (calendar-sync--set-calendar-state "proton" '(:status syncing))
-      (calendar-sync--sync-calendar '(:name "proton" :url "https://x/y.ics"
-                                            :file "/tmp/c.org"))
-      (should (null api-calls))
-      (should (null ics-calls)))))
+  (test-cs-syncing--with-fresh-state
+    (let ((api-calls '()) (ics-calls '()))
+      (cl-letf (((symbol-function 'calendar-sync--sync-calendar-api)
+                 (lambda (cal) (push cal api-calls)))
+                ((symbol-function 'calendar-sync--sync-calendar-ics)
+                 (lambda (cal) (push cal ics-calls))))
+        (calendar-sync--set-calendar-state "proton" '(:status syncing))
+        (calendar-sync--sync-calendar '(:name "proton" :url "https://x/y.ics"
+                                              :file "/tmp/c.org"))
+        (should (null api-calls))
+        (should (null ics-calls))))))
 
 (ert-deftest test-calendar-sync--sync-calendar-dispatches-when-idle ()
   "Boundary: an idle calendar (no in-flight status) still dispatches normally."
-  (test-cs-syncing--reset)
-  (let ((ics-calls '()))
-    (cl-letf (((symbol-function 'calendar-sync--sync-calendar-ics)
-               (lambda (cal) (push cal ics-calls))))
-      (calendar-sync--sync-calendar '(:name "proton" :url "https://x/y.ics"
-                                            :file "/tmp/c.org"))
-      (should (= 1 (length ics-calls))))))
+  (test-cs-syncing--with-fresh-state
+    (let ((ics-calls '()))
+      (cl-letf (((symbol-function 'calendar-sync--sync-calendar-ics)
+                 (lambda (cal) (push cal ics-calls))))
+        (calendar-sync--sync-calendar '(:name "proton" :url "https://x/y.ics"
+                                              :file "/tmp/c.org"))
+        (should (= 1 (length ics-calls)))))))
+
+;;; Isolation guard
+
+(ert-deftest test-calendar-sync--syncing-state-does-not-leak ()
+  "Error: state written inside the macro is gone once it returns.
+Pins the isolation itself.  Without it a test marking a calendar syncing
+leaves that status set for every later test in the same process, which is
+exactly what broke the sibling dispatch test."
+  (test-cs-syncing--with-fresh-state
+    (calendar-sync--set-calendar-state "leak-probe" '(:status syncing))
+    (should (calendar-sync--syncing-p "leak-probe")))
+  (should-not (calendar-sync--syncing-p "leak-probe")))
 
 ;;; load-state sanitize: a persisted `syncing' status is cleared on load
 
 (ert-deftest test-calendar-sync--load-state-clears-stale-syncing ()
   "Error: a `syncing' status persisted before a crash is reset on load, so the
 in-flight guard cannot skip that calendar forever in the new session."
-  (test-cs-syncing--reset)
-  (let* ((dir (make-temp-file "cs-state-" t))
-         (calendar-sync--state-file (expand-file-name "state.el" dir)))
-    (unwind-protect
-        (progn
-          (with-temp-file calendar-sync--state-file
-            (prin1 '((timezone-offset . nil)
-                     (calendar-states . (("google" . (:status syncing)))))
-                   (current-buffer)))
-          (calendar-sync--load-state)
-          (should-not (calendar-sync--syncing-p "google")))
-      (delete-directory dir t))))
+  (test-cs-syncing--with-fresh-state
+    (let* ((dir (make-temp-file "cs-state-" t))
+           (calendar-sync--state-file (expand-file-name "state.el" dir)))
+      (unwind-protect
+          (progn
+            (with-temp-file calendar-sync--state-file
+              (prin1 '((timezone-offset . nil)
+                       (calendar-states . (("google" . (:status syncing)))))
+                     (current-buffer)))
+            (calendar-sync--load-state)
+            (should-not (calendar-sync--syncing-p "google")))
+        (delete-directory dir t)))))
 
 (provide 'test-calendar-sync--syncing-p)
 ;;; test-calendar-sync--syncing-p.el ends here
diff --git a/tests/test-validate-el-hook.bats b/tests/test-validate-el-hook.bats
index 2dbcae79..43c3569c 100644
--- a/tests/test-validate-el-hook.bats
+++ b/tests/test-validate-el-hook.bats
@@ -1,19 +1,21 @@
 #!/usr/bin/env bats
-# Tests for .claude/hooks/validate-el.sh — specifically the auto-test cap.
+# Tests for .claude/hooks/validate-el.sh — the auto-test runner.
 #
-# The hook runs the tests matching an edited file, but only when the match
-# count is between 1 and MAX_AUTO_TEST_FILES.  Above the cap the whole block
-# was skipped with no else branch: nothing printed, exit 0, indistinguishable
-# from a passing run.  That is live for the three largest families here
-# (calendar-sync 63 test files, music 45, ai-term 35), so every edit to those
-# modules ran parens and byte-compile and zero tests, silently.
+# The runner used to skip entirely above MAX_AUTO_TEST_FILES=20, with no else
+# branch: nothing printed, exit 0, indistinguishable from a passing run.  That
+# was live for the three largest families here (calendar-sync 63 test files,
+# music 45, ai-term 35), so every edit to those ran parens and byte-compile and
+# zero tests, silently.
 #
-# The cap itself is fine — running 63 files per keystroke is not wanted.  The
-# defect is the silence, so these tests assert the skip announces itself and
-# names what to run.
+# The cap was removed rather than made loud, because its premise did not hold.
+# Measured on this machine, running a whole family takes about a second:
+# ai-term 208 tests in 1.0s, music 403 in 1.7s, calendar-sync 633 in 0.9s.  It
+# was also concealing a real cross-test pollution bug in calendar-sync that
+# only appears when that family runs in one process.
 #
-# Each test builds a synthetic project in BATS_TEST_TMPDIR and points
-# CLAUDE_PROJECT_DIR at it, so nothing runs against the real tree.
+# These tests pin that no file count is skipped.  Each builds a synthetic
+# project in BATS_TEST_TMPDIR and points CLAUDE_PROJECT_DIR at it, so nothing
+# runs against the real tree.
 
 setup() {
     HOOK="${BATS_TEST_DIRNAME}/../.claude/hooks/validate-el.sh"
@@ -23,8 +25,7 @@ setup() {
     printf '(provide (quote widget))\n' > "$PROJ/modules/widget.el"
 }
 
-# Create N test files matching the widget stem.  Each is trivially green so a
-# run below the cap succeeds and the only variable is the count.
+# N green test files matching the widget stem.
 make_tests() {
     local n="$1" i
     for ((i = 1; i <= n; i++)); do
@@ -33,41 +34,64 @@ make_tests() {
     done
 }
 
+# One failing test file, to prove the run is real rather than merely quiet.
+make_failing_test() {
+    printf '(require (quote ert))\n(ert-deftest test-widget-bad () (should nil))\n' \
+        > "$PROJ/tests/test-widget-bad.el"
+}
+
 hook_input() {
     printf '{"tool_input":{"file_path":"%s"}}' "$PROJ/modules/widget.el"
 }
 
+run_hook() {
+    run bash -c "$(printf '%q' "$HOOK") <<< '$(hook_input)'"
+}
+
 # ------------------------------- Normal cases -------------------------------
 
-@test "under the cap: runs the tests and stays quiet on success" {
+@test "a small family runs and passes quietly" {
     make_tests 3
-    run bash -c "$(printf '%q' "$HOOK") <<< '$(hook_input)'"
+    run_hook
     [ "$status" -eq 0 ]
-    [[ "${output,,}" != *"skipped"* ]]
+}
+
+@test "a failing test blocks, so a quiet pass means the tests really ran" {
+    make_tests 3
+    make_failing_test
+    run_hook
+    [ "$status" -eq 2 ]
+    [[ "$output" == *"TESTS FAILED"* ]]
 }
 
 # ------------------------------ Boundary cases ------------------------------
 
-@test "exactly at the cap: still runs the tests" {
+@test "at the old cap of 20 files: runs" {
     make_tests 20
-    run bash -c "$(printf '%q' "$HOOK") <<< '$(hook_input)'"
+    run_hook
     [ "$status" -eq 0 ]
-    [[ "${output,,}" != *"skipped"* ]]
 }
 
-# -------------------------------- Error cases -------------------------------
-
-@test "over the cap: says it skipped rather than exiting silently" {
+@test "past the old cap: still runs, no longer skipped" {
     make_tests 21
-    run bash -c "$(printf '%q' "$HOOK") <<< '$(hook_input)'"
-    # Must not fail the edit — the cap is deliberate, the silence is not.
+    run_hook
     [ "$status" -eq 0 ]
-    [[ "${output,,}" == *"skipped"* ]]
+    [[ "${output,,}" != *"skipped"* ]]
 }
 
-@test "over the cap: names the count and how to run them" {
-    make_tests 21
-    run bash -c "$(printf '%q' "$HOOK") <<< '$(hook_input)'"
-    [[ "$output" == *"21"* ]]
-    [[ "$output" == *"make test-file"* ]]
+@test "well past the old cap: a failure in file 63 is still caught" {
+    # The regression this guards: at 63 files the runner used to skip, so a red
+    # test in a big family reported clean.  calendar-sync is exactly this size.
+    make_tests 63
+    make_failing_test
+    run_hook
+    [ "$status" -eq 2 ]
+    [[ "$output" == *"TESTS FAILED"* ]]
+}
+
+# -------------------------------- Error cases -------------------------------
+
+@test "no matching tests: exits clean without running anything" {
+    run_hook
+    [ "$status" -eq 0 ]
 }
-- 
cgit v1.2.3