test(sync,e2e): replace busy-poll loops with wall-clock waits - #25
Merged
Conversation
- Move design_specs/ into docs/design_specs/ and fix relative links in the frontend spec to match the new location - Remove obsolete REMAINING_WORK planning docs - Anchor graphify-out/ ignore rule to repo root - Update sync/logo SVG assets and add sync_nas asset - Regenerate graphify-out/ knowledge-graph artifacts Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix the timing-sensitive flakes that fail under CI/CPU load (red on main):
the sync tests drove a background worker then busy-polled a FIXED iteration
count (range(200)/range(400)/range(600) at 10ms = ~2-6s budgets). A loaded
worker thread starves and the budget elapses before the state settles, so
the loop pytest.fail()s despite correct behavior.
Replace 23 such loops across tests/unit/sync/{test_nas_client,
test_nas_client_extra}.py and tests/integration/test_nas_sync.py with two
shared helpers in tests/unit/sync/_helpers.py:
- wait_until(predicate, timeout=30): generic wall-clock-bounded await.
- wait_for_job_state(client, job_id, target, timeout=30): the common
queue-state shape; returns the row, names the last state on timeout.
The budget is now a generous TIME ceiling (irrelevant when fast, robust
when slow) rather than a magic iteration count. Also deletes the duplicate
local _wait_for_state helper in test_nas_client.py.
e2e: tests/e2e/test_flow_06_problems.py read row text immediately after
wait_for(visible), catching the intermediate 'Active' before the override
badge re-rendered to 'Override active'. Use Playwright's auto-retrying
expect(...).to_contain_text(...) instead.
No production code changed. Verified: sync unit 116 passed x3, integration
11 passed x2, ruff + format clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Xander-git
added a commit
that referenced
this pull request
May 31, 2026
The two happy-path tests waited for the queue ROW to reach a terminal state (wait_for_job_state) and then immediately read creation.json's sync_status. That field is stamped by a separate async step (_mark_synced / _mark_cleaned) that lags the queue-row transition, so a loaded CI runner observed the pre-stamp 'pending' value and the assert failed (a residual instance of the #25 timing-flake class -- these two were the spots #25 did not fully convert). Poll the file itself with wait_until until sync_status reaches the asserted value, instead of reading it the instant the row goes terminal. No production code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Xander-git
added a commit
that referenced
this pull request
May 31, 2026
…fy (#27) * feat(dist): tag-based asset names; pre-release builds without notify Release-asset naming: per-OS installers are now named ExLabWizard_v<release-tag>.<ext> (e.g. ExLabWizard_v0.2.0.exe/.dmg/.AppImage). The name is driven by the release tag with a leading v/V normalised, so both v0.2.0 and 0.2.0 produce ExLabWizard_v0.2.0 (never _vv0.2.0). Non-release builds (dispatch/PR) fall back to the package __version__. Raw onedir archives keep an OS suffix (ExLabWizard_v<tag>-<suffix>.{zip,tar.gz}) because macOS and Linux both emit .tar.gz and would otherwise collide as release assets. The Inno .iss takes a /DOutputBaseName define; upload + release globs updated to ExLabWizard_v*. Pre-releases: the `published` activity type already fires for pre-releases, so publishing a pre-release builds + attaches the full installer set (test an RC before shipping). No trigger change; documented in the workflow header. Pre-releases do NOT notify: the startup checker polls releases/latest, which GitHub defines as the most recent non-prerelease, non-draft release -- so a pre-release never prompts an upgrade. Added a defensive guard in fetch_latest_tag (skip payloads flagged prerelease/draft) + two unit tests so the requirement is encoded in our code, not just implied by the endpoint. README updated for the new asset names and the pre-release behaviour. Tag-name input is routed through env (not interpolated into the run body) to avoid shell injection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sync): wait for creation.json status, not just queue state The two happy-path tests waited for the queue ROW to reach a terminal state (wait_for_job_state) and then immediately read creation.json's sync_status. That field is stamped by a separate async step (_mark_synced / _mark_cleaned) that lags the queue-row transition, so a loaded CI runner observed the pre-stamp 'pending' value and the assert failed (a residual instance of the #25 timing-flake class -- these two were the spots #25 did not fully convert). Poll the file itself with wait_until until sync_status reaches the asserted value, instead of reading it the instant the row goes terminal. No production code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the timing-sensitive test flakes that were failing CI under load (red on
main, surfaced by the checks on #23). These are test-only changes — no production code is touched. Implements options B (event-driven/time-bounded waits) and C (Playwright auto-retry) from the follow-up plan.Root cause
The sync tests drive a background worker thread and then busy-poll a fixed iteration count for a state change:
The worker runs at
worker_poll_interval_s=0.005. On a loaded runner the worker thread doesn't get scheduled often enough to finish its retry/reconcile cycle within the fixed budget, so the loop exhausts and fails despite correct behavior. The e2e test had the same class of bug: it read row text right afterwait_for(visible), catching the transient'Active'before the override badge re-rendered to'Override active'.Changes (B — sync waits)
Two shared helpers added to
tests/unit/sync/_helpers.py:wait_until(predicate, *, timeout=30, interval=0.02, message=...)— generic wall-clock-bounded await on an async predicate.wait_for_job_state(client, job_id, target, *, timeout=30)— the common queue-state shape; returns the matching row, and on timeout raises naming the last observed state (strictly more informative than the oldpytest.failstrings).The budget is now a generous time ceiling (30s — irrelevant when the worker is responsive, robust when it's slow) instead of a magic iteration count. 23 busy-poll loops were replaced across:
tests/unit/sync/test_nas_client.py(6 — incl. deleting the duplicate local_wait_for_statehelper)tests/unit/sync/test_nas_client_extra.py(8)tests/integration/test_nas_sync.py(9 — queue-state loops →wait_for_job_state; filesystem/sync_state predicates →wait_until)Negative assertions and post-loop property checks were preserved verbatim; only the initial busy-wait was swapped.
Changes (C — e2e)
tests/e2e/test_flow_06_problems.py: replacedwait_for(visible)+inner_text()assertions with Playwright's auto-retryingexpect(...).to_contain_text(..., timeout=5_000).Test plan
ruff check+ruff format --checkclean on all 5 filespytest tests/unit/sync -p no:randomly— 116 passed, run 3× (the previously-flaky tests, now stable)pytest tests/integration/test_nas_sync.py -p no:randomly— 11 passed, run 2×src/changesTogether with #23 this should bring the
qcworkflow fully green onmain.🤖 Generated with Claude Code