docs(spec+plan): wait_for_ready_test_pattern (replace time.sleep with polling)

2026-06-05 22:45:14 -04:00
parent 85cd34683a
commit b692353e98
2 changed files with 303 additions and 0 deletions
@@ -0,0 +1,112 @@
+# wait_for_ready_test_pattern_20260605 — Design
+
+**Date:** 2026-06-05
+**Status:** Draft
+**Track:** wait_for_ready_test_pattern_20260605 (sub-project of v2)
+
+## Problem Statement
+
+Two failing live_gui tests use `time.sleep(N)` to wait for asynchronous GUI operations to complete:
+
+- `tests/test_workspace_profiles_sim.py` — `time.sleep(2.0)` after save and after load; `time.sleep(1.0)` after each set_value.
+- `tests/test_auto_switch_sim.py` — `time.sleep(1)` after each `push_event`.
+
+Fixed sleeps are a fragile test pattern:
+- On slow machines the sleep may be insufficient; the assertion runs before the operation completes.
+- On fast machines the sleep is wasted; the test takes longer than necessary.
+- Tests that pass with `time.sleep(2.0)` in CI may fail on a developer machine with different load.
+
+After the state-sync fix (`live_gui_state_sync_20260605`) lands, these tests should pass at the current 2-second sleep. **But the test pattern is still wrong** — the tests should poll for completion, not assume timing.
+
+## Design
+
+### Approach: Migrate `time.sleep` to a wait-for-ready helper
+
+`src/api_hook_client.py` already exposes `wait_for_event(event_type, timeout)` and `get_value(item)`. The tests can use these directly.
+
+**Hypothetical example — the current pattern:**
+
+```python
+client.set_value('ui_separate_tier1', True)
+time.sleep(1.0)
+client.push_event("custom_callback", {"callback": "save_workspace_profile", "args": ["test_restore", "project"]})
+time.sleep(2.0)  # HOPE the save completes within 2s
+client.set_value('ui_separate_tier1', False)
+time.sleep(1.0)
+client.push_event("custom_callback", {"callback": "load_workspace_profile", "args": ["test_restore"]})
+time.sleep(2.0)  # HOPE the load completes within 2s
+assert client.get_value('ui_separate_tier1') is True
+```
+
+**Migrated pattern:**
+
+```python
+def wait_for_save_completion(client, profile_name, timeout=5.0):
+    """Poll until the saved profile appears in the workspace profiles."""
+    import time
+    deadline = time.time() + timeout
+    while time.time() < deadline:
+        profiles = client.get_value('workspace_profiles') or {}
+        if profile_name in profiles:
+            return
+        time.sleep(0.1)
+    raise TimeoutError(f"Save did not complete within {timeout}s")
+
+def wait_for_load_completion(client, item, expected, timeout=5.0):
+    """Poll until the item's value matches expected."""
+    import time
+    deadline = time.time() + timeout
+    while time.time() < deadline:
+        if client.get_value(item) == expected:
+            return
+        time.sleep(0.1)
+    raise TimeoutError(f"Load did not apply {item}={expected} within {timeout}s")
+
+client.set_value('ui_separate_tier1', True)
+# No sleep needed; set_value returns when the value is set on the controller
+client.push_event("custom_callback", {"callback": "save_workspace_profile", "args": ["test_restore", "project"]})
+wait_for_save_completion(client, "test_restore")
+client.set_value('ui_separate_tier1', False)
+client.push_event("custom_callback", {"callback": "load_workspace_profile", "args": ["test_restore"]})
+wait_for_load_completion(client, 'ui_separate_tier1', True)
+```
+
+### Why this approach
+
+- **Polling, not fixed sleeps**: 100ms poll interval is responsive without busy-waiting.
+- **Generous timeouts**: 5s default is well over the typical ~100ms operation; catches genuine hangs.
+- **Reusable helpers**: `wait_for_save_completion` and `wait_for_load_completion` are simple and can be added to a shared test helper module.
+- **Failure messages are clear**: TimeoutError explicitly says which operation timed out.
+
+### Alternatives considered
+
+- **A2: Add wait_for_X helpers to ApiHookClient itself.** Rejected: ApiHookClient should remain a thin transport; test-helper logic doesn't belong there. Keep helpers in `tests/conftest.py` or a `tests/helpers.py` module.
+- **A3: Use `wait_for_event` exclusively.** The Hook API's `wait_for_event` listens for events the GUI emits. save/load may not emit events in a way the test can match. Polling `get_value` is more direct.
+
+## File Changes
+
+### Modify: `tests/test_workspace_profiles_sim.py`
+
+Replace `time.sleep(...)` with `wait_for_save_completion` and `wait_for_load_completion` calls. Add the helper functions at the top of the file (or import from a shared helper).
+
+### Modify: `tests/test_auto_switch_sim.py`
+
+Replace `time.sleep(...)` with similar polling helpers.
+
+### Optionally: Create: `tests/helpers.py`
+
+If multiple tests need the same helpers, extract them to a shared module. For now, keep them inline (2 tests, ~30 lines of helpers total).
+
+## Risk Assessment
+
+| Risk | Likelihood | Impact | Mitigation |
+|---|---|---|---|
+| The polling masks a slow operation that's now flaky | Low | Medium | Generous 5s timeout; if a test times out, the test message points to which operation |
+| Helper functions added in 2 places diverge | Medium | Low | If 3+ tests need the same helper, extract to `tests/helpers.py` |
+
+## Out of Scope
+
+- **State sync fix** — separate track (`live_gui_state_sync_20260605`).
+- **prior_session test** — separate track (`prior_session_test_harden_20260605`).
+- **Migrating other live_gui tests that use `time.sleep`** — out of scope for now. Track as a follow-up if more flakes appear.
+- **Replacing `time.sleep` with `asyncio.sleep`** — out of scope; the live_gui tests are sync, and the GUI event queue is sync.