tests: wait for the thing, don't sleep and hope

test_trace_ui spent 18.8 of its 35.2 seconds in flat pilot.pause() calls placed
to let an async seek land. Two loops were most of it: 6 iterations at 0.5s and
28 at 0.3s, 11.4s of sleeping to check that a step moves the cursor.

They are condition waits now. The questions are unchanged -- does the listing
cursor reach the pc, does the pseudocode cursor follow -- but they cost what
they cost instead of a fixed budget. The second loop settles on something that
does NOT presuppose the answer (the listing cursor arriving, and the trail map
belonging to the loaded function): waiting on 'is this pc mapped' would have
burned the timeout on every unmapped instruction, about half of them, and come
out slower than the sleep it replaced.

35.2s -> 20.9s, 39 checks, stable over repeated runs.

tests/_fixtures.py collects the staging both this suite and test_scenarios need
-- scratch copy, seeded from a golden .i64 nothing writes back to -- which was
private to test_scenarios. Worth saying plainly: on targets/echo the seeding is
worth 0.19s, not the analysis time I assumed when I went looking. It is shared
for the deduplication and for whatever gets pointed at a bigger binary.

4 files changed, 140 insertions, 46 deletions

diff --git a/tests/_fixtures.py b/tests/_fixtures.py
new file mode 100644
index 0000000..766ecd5
--- /dev/null
+++ b/tests/_fixtures.py
@@ -0,0 +1,89 @@
+"""Shared setup for the suites that need a real analysed binary.
+
+Two things every IDA suite has to get right, and only test_scenarios did:
+
+**Work on a scratch copy.** Opening a binary writes a `.i64` beside it, and the
+suites EDIT it -- they define code, undefine items, rename and comment, and IDA
+saves that. Run against the tracked target and each run inherits the last one's
+damage: a scenario started failing with no code change because an earlier one
+had undefined an instruction. A suite whose result depends on its own history
+can't be trusted to accuse the code.
+
+**Seed from a golden database.** Auto-analysis is the bulk of a suite's runtime
+(`targets/echo` is ~30s of it) and it produces the same answer every time. Doing
+it once and keeping the result as `<binary>.pristine.i64` -- which nothing ever
+writes back to -- turns that into a file copy.
+
+    with staged("targets/echo") as target:
+        app = IdaTui(open_path=target, ...)
+
+The cache is rebuilt whenever it is older than the binary, so editing a target
+doesn't silently test the previous one. `.pristine.i64` is gitignored.
+"""
+from __future__ import annotations
+
+import contextlib
+import os
+import shutil
+import tempfile
+
+
+def cache_path(binary: str) -> str:
+    return binary + ".pristine.i64"
+
+
+def cache_is_fresh(binary: str) -> bool:
+    c = cache_path(binary)
+    return os.path.exists(c) and os.path.getmtime(c) >= os.path.getmtime(binary)
+
+
+async def build_pristine(binary: str, cache: str, app_factory) -> None:
+    """Analyse ``binary`` once and keep the database as a golden copy.
+
+    ``app_factory(path)`` builds the IdaTui -- passed in so this module needs no
+    import of the app (and so a suite can hand over its own load options).
+    """
+    print(f"  (building pristine database for {os.path.basename(binary)}\u2026)")
+    app = app_factory(binary)
+    async with app.run_test(size=(140, 44)) as pilot:
+        for _ in range(6000):
+            await pilot.pause(0.05)
+            if app._func_index is not None and app._func_index.complete:
+                break
+        app.program.client.call("idb_save", timeout=600.0)
+    db = binary + ".i64"
+    if os.path.exists(db):
+        shutil.copy2(db, cache)
+
+
+@contextlib.contextmanager
+def scratch_copy(binary: str, prefix: str = "idatui-test-"):
+    """A temp-dir copy of ``binary``, seeded from the pristine cache if there is
+    a fresh one. Yields the copy's path; the directory goes away after.
+
+    Does NOT build the cache (that needs an app and an event loop) -- a suite
+    that wants one calls :func:`build_pristine` first. Without a cache this is
+    still correct, just slow: the worker analyses from scratch.
+    """
+    with tempfile.TemporaryDirectory(prefix=prefix) as d:
+        target = os.path.join(d, os.path.basename(binary))
+        shutil.copy2(binary, target)
+        if cache_is_fresh(binary):
+            shutil.copy2(cache_path(binary), target + ".i64")
+        yield target
+
+
+@contextlib.asynccontextmanager
+async def staged(binary: str, app_factory=None, prefix: str = "idatui-test-"):
+    """:func:`scratch_copy`, building the pristine cache first if it's missing.
+
+    This is what a suite wants: one call, and the analysis is paid once ever
+    rather than once per run.
+    """
+    if app_factory is not None and not cache_is_fresh(binary):
+        with tempfile.TemporaryDirectory(prefix=prefix) as seed_dir:
+            seed = os.path.join(seed_dir, os.path.basename(binary))
+            shutil.copy2(binary, seed)
+            await build_pristine(seed, cache_path(binary), app_factory)
+    with scratch_copy(binary, prefix) as target:
+        yield target
diff --git a/tests/run.py b/tests/run.py
index ca0a319..4920e54 100755
--- a/tests/run.py
+++ b/tests/run.py
@@ -20,6 +20,14 @@ first time someone adds a test, **each test file declares it**::
 modules run their suite at import time). A test file with no marker is a hard
 error, so a new test can't quietly join the fast set and start needing IDA.
 
+Deliberately **serial**. Running the IDA suites concurrently looks like the
+obvious win (they are independent processes with their own worker and temp dir,
+on a 12-core box) and it is measurably a loss: 4 at a time took the suite from
+153s to 296s and killed three of them with broken-pipe worker failures --
+thumb_ui alone went 10.4s to 287.8s. idalib contends hard enough that the extra
+processes only starve each other, and a starved worker gets reaped mid-analysis,
+which reads as a flaky test rather than as load. Don't re-add --jobs.
+
 Usage::
 
     python3 tests/run.py              # everything
diff --git a/tests/test_scenarios.py b/tests/test_scenarios.py
index 68ebfa3..0dbb0bd 100644
--- a/tests/test_scenarios.py
+++ b/tests/test_scenarios.py
@@ -24,12 +24,12 @@ import asyncio
 import fnmatch
 import os
 import re
-import shutil
 import sys
-import tempfile
 import traceback
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
+from _fixtures import staged  # noqa: E402
 from idatui.app import (  # noqa: E402
     ConfirmScreen, DecompView, FunctionsPanel, GraphView, HexView, IdaTui,
     HelpScreen, ListingView, QuitScreen, StringsPalette, StructEditor,
@@ -3137,25 +3137,6 @@ async def s_graph_sticky(c: Ctx):
 # --------------------------------------------------------------------------- #
 # Runner
 # --------------------------------------------------------------------------- #
-async def _build_pristine(binary, cache):
-    """Analyse ``binary`` once and keep the resulting database as a golden copy.
-
-    Costs one full analysis, then every later run starts from it instead of
-    re-analysing.
-    """
-    print(f"  (building pristine database for {os.path.basename(binary)}\u2026)")
-    app = IdaTui(open_path=binary, keepalive=False)
-    async with app.run_test(size=(140, 44)) as pilot:
-        for _ in range(6000):
-            await pilot.pause(0.05)
-            if app._func_index is not None and app._func_index.complete:
-                break
-        app.program.client.call("idb_save", timeout=600.0)
-    db = binary + ".i64"
-    if os.path.exists(db):
-        shutil.copy2(db, cache)
-
-
 async def run(binary, only=None):
     # The suite EDITS the database — it defines code, undefines items, renames
     # and comments — and IDA saves those edits. Run that against the tracked
@@ -3167,15 +3148,8 @@ async def run(binary, only=None):
     #
     # So: work on a scratch copy, seeded from a golden database that nothing
     # ever writes back to.
-    with tempfile.TemporaryDirectory(prefix="idatui-pilot-") as scratch:
-        target = os.path.join(scratch, os.path.basename(binary))
-        shutil.copy2(binary, target)
-        cache = binary + ".pristine.i64"
-        if not (os.path.exists(cache)
-                and os.path.getmtime(cache) >= os.path.getmtime(binary)):
-            await _build_pristine(target, cache)
-        if os.path.exists(cache):
-            shutil.copy2(cache, target + ".i64")
+    async with staged(binary, lambda p: IdaTui(open_path=p, keepalive=False),
+                      prefix="idatui-pilot-") as target:
         await _run_on(target, only)
 
 
diff --git a/tests/test_trace_ui.py b/tests/test_trace_ui.py
index d222eab..bb016eb 100644
--- a/tests/test_trace_ui.py
+++ b/tests/test_trace_ui.py
@@ -11,15 +11,15 @@ the trace is part of this repo.
 NEEDS_IDA = True
 import asyncio
 import os
-import shutil
 import subprocess
 import sys
-import tempfile
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
 
 from textual.widgets import Input, OptionList, Static  # noqa: E402
 
+from _fixtures import staged  # noqa: E402
 from idatui.app import (DecompView, IdaTui, ListingView,  # noqa: E402
                         RegWriteScreen, TraceDock)
 
@@ -63,11 +63,14 @@ def make_trace(tmp, binary):
 
 async def run() -> int:
     binary = os.path.join(REPO, "targets", "echo")
-    with tempfile.TemporaryDirectory() as tmp:
-        # Scratch copy: opening a binary writes a database beside it, and the
-        # suite must not edit anything tracked.
-        target = os.path.join(tmp, "echo")
-        shutil.copy2(binary, target)
+    # Scratch copy seeded from the golden database (tests/_fixtures.py):
+    # opening a binary writes a .i64 beside it and the suite must not touch
+    # anything tracked. On echo the seeding saves only ~0.2s (it analyses fast);
+    # it is here so a suite pointed at a bigger target doesn't pay for analysis
+    # on every run.
+    async with staged(binary, lambda p: IdaTui(open_path=p, keepalive=False),
+                      prefix="idatui-traceui-") as target:
+        tmp = os.path.dirname(target)
         log = make_trace(tmp, target)
         if not log:
             print(f"  skip: could not record a trace (tracer at {TRACER})")
@@ -152,7 +155,9 @@ async def run() -> int:
             # image would have nothing to show.
             dock = app.query_one(TraceDock)
             app._seek(min(60, t.length - 1))
-            await pilot.pause(0.8)
+            await wait(lambda: "stack (" in
+                       str(dock.query_one("#trace-stack", Static).render()),
+                       pilot, 5)
             stack = str(dock.query_one("#trace-stack", Static).render())
             check("the dock shows the stack at this timestamp",
                   "stack (" in stack and len(stack.splitlines()) > 4, stack[:60])
@@ -175,7 +180,9 @@ async def run() -> int:
             # Stepping must move the memory view with time.
             before = stack
             app._seek(min(80, t.length - 1))
-            await pilot.pause(0.8)
+            await wait(lambda: str(dock.query_one("#trace-stack",
+                                                  Static).render()) != before,
+                       pilot, 5)
             check("and it follows as you move through time",
                   str(dock.query_one("#trace-stack", Static).render()) != before)
 
@@ -184,7 +191,7 @@ async def run() -> int:
             # program that's almost everything and says nothing. The last/next
             # few dozen steps say how you got here and where you're going.
             app._seek(min(40, t.length - 1))
-            await pilot.pause(0.6)
+            await wait(lambda: bool(lst.trail), pilot, 5)
             trail = lst.trail
             kinds = {k for k in trail.values()}
             check("the listing is painted with an execution trail",
@@ -219,7 +226,7 @@ async def run() -> int:
                 dec = app.query_one(DecompView)
                 check("pseudocode is available for the traced function", got)
                 app._seek(first + 12)
-                await pilot.pause(1.0)
+                await wait(lambda: len(dec.trail) > 2, pilot, 5)
                 check("pseudocode lines are painted with the trail",
                       len(dec.trail) > 2, f"{len(dec.trail)} lines")
                 now = [i for i, k in dec.trail.items() if k == "now"]
@@ -252,7 +259,8 @@ async def run() -> int:
             # navigation though: both panes show the same instant, so the
             # listing cursor must sit on the current instruction.
             app.action_toggle_split()
-            await pilot.pause(2.0)
+            await wait(lambda: app._split and lst.display
+                       and app.query_one(DecompView).display, pilot, 10)
             if not app._split:
                 check("split view toggled on", False)
             else:
@@ -260,8 +268,11 @@ async def run() -> int:
                 tracked = 0
                 for k in range(2, 8):
                     app._seek(base + k)
-                    await pilot.pause(0.5)
-                    if lst._cursor_ea() == t.ip(app._t):
+                    # Wait for the cursor to arrive rather than sleeping a flat
+                    # 0.5s and hoping. Same question -- does the listing follow
+                    # the pc? -- but it costs what it costs instead of 3s.
+                    if await wait(lambda: lst._cursor_ea() == t.ip(app._t),
+                                  pilot, 5):
                         tracked += 1
                 check("stepping in split moves the listing cursor to the pc",
                       tracked == 6, f"{tracked}/6 steps tracked")
@@ -280,11 +291,23 @@ async def run() -> int:
                 mapped = missed = 0
                 for k in range(2, 30):
                     app._seek(base + k)
-                    await pilot.pause(0.3)
                     pc = t.ip(app._t)
+                    # Settle on something that does NOT presuppose the answer:
+                    # the listing cursor reaching the pc (checked just above)
+                    # and the trail map belonging to the loaded function. Waiting
+                    # on `pc in _trail_line_of` instead would burn the timeout on
+                    # every unmapped instruction -- about half of them -- and be
+                    # slower than the flat sleep it replaces.
+                    await wait(lambda: lst._cursor_ea() == pc
+                               and app._trail_map_ea == dec.loaded_ea, pilot, 5)
                     if app._trail_map_ea == dec.loaded_ea and pc in app._trail_line_of:
                         mapped += 1
-                        if dec.cursor != app._trail_line_of[pc]:
+                        # Mapped: the pseudocode cursor is expected, so it's fair
+                        # to wait for it -- and this is the assertion, so the
+                        # miss is still counted if it never arrives.
+                        line = app._trail_line_of[pc]
+                        await wait(lambda: dec.cursor == line, pilot, 3)
+                        if dec.cursor != line:
                             missed += 1
                 check("the pseudocode cursor follows every mapped instruction",
                       mapped > 3 and missed == 0,