test(ui): wait for the export's notification, not for the file it wrote - #128
Merged
Conversation
`export_to` and the two e2e exports waited on the archive appearing. That is satisfied by the *worker thread* — `export_model` finishes its atomic os.replace and only afterwards posts completion — so the wait could return with "Export complete" still travelling through the bus. Two ways that bit, both seen on macOS py3.13 in CI while every other leg passed: - a test asserting `notify.titles` on the next line read `[]`, one drain early; - `import_from` waits for "a notification that wasn't there before", and the export's late arrival satisfied it — so the helper returned while the import worker was still running and the row it had created read back with `model_path` still None. Waiting on the notification fixes both, and keeps the file assertion as a separate check rather than a synchronisation point. The regression test drives a deliberately slow export so the window is deterministic instead of a matter of which runner you land on; it fails with an IndexError against the old helper. Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
jimisola
enabled auto-merge (squash)
August 20, 2026 08:44
sjseth
approved these changes
Aug 26, 2026
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.
What & Why
Found on #126 (the
setup-uvv10 bump), which had nothing to do with it:pytest (macos-latest, py3.13)went red while py3.12 and py3.14 on the same runner passed, and a plain re-run then went green. Two failures, one root cause, and it is a race that has been in the suite the whole time — it just needs the timing to land badly.The cause
dest.exists()is satisfied by the worker thread, mid-flight.export_modelwrites<dest>.tmpand then does an atomicos.replace, so the archive appears the moment the worker is done writing — while_on_exported'snotify("Export complete", …)is still on its way back through the bus to the main thread. The wait returns early, with a notification pending.That breaks two things, in different ways:
window.notify.titleson the very next line, one drain too soon, and reads[].import_fromwaits forlen(window.notify.calls) > before, wherebeforewas captured while the export's notification was still queued. The export's own late-arriving notify satisfies that condition, so the helper returns before the import worker has finished.import_modelsetsmodel_pathduring extraction, so the row reads back withmodel_pathstillNone.The second failure never mentions exports at all, which is why it looked like two unrelated flakes.
The fix
Wait for the completion that the UI actually reports, and keep the file as an assertion rather than a synchronisation point:
Three sites: the
export_tohelper intest_models.pyand the two exports intest_e2e.py.import_fromis left alone — its wait was the right shape all along, it was just being fed a stalebefore.Deliberately not touched:
test_models.py's delete test still waits onnot paths.model_dir(...).exists(). That worker isrun_worker(lambda: shutil.rmtree(...))with noon_done, so the directory vanishing is the only completion signal there is. Different situation, correctly written.The regression test
test_export_to_waits_for_the_notification_not_the_archivemakes the window deterministic instead of leaving it to which runner you land on: it stubsexport_modelwith one that writes the archive and then takes 250 ms, so the file exists long before the notification does. Against the old helper it fails withIndexError: list index out of range— there is no notification at all yet. Against the new one it passes.The sleep is simulated slow work inside the export, not the test waiting on an async result;
drain_untilstill does all the waiting, so this doesn't reintroduce what CLAUDE.md §5 rules out.Test Plan
pytest— 1448 passed.ruff check,ruff format --check,ty check— clean.export_toto the old wait and confirmed the new test fails; restored it and confirmed it passes.Not reproducible on Linux, which is the point — on this machine
drain_untilhappens to deliver the notification in the same iteration that the file appears. macOS scheduled it differently and CI found it.Author checklist
CLAUDE.mdchange — this is a test-helper fix, no architecture or boundary moved