Repository navigation
M3: make two open-file tests read the file on the frame thread (red CI on main) - #1897
philip-tam wants to merge 1 commit into
Conversation
a_tool_chosen_with_no_document_asks_for_one_and_runs_on_it and the_open_recent_menu_lists_files_and_opens_one open a file and assert after a fixed number of frames. Since storytold#1493 a running app reads and prepares the file on a worker thread that finishes at its own pace, so the assertion races it: the first fails every time on a development Mac and in CI, the second some of the time. Both tests now set run_inline, as the OCR and action tests do, so the file is read and prepared on the frame thread and the steps are enough. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
FredApps
left a comment
There was a problem hiding this comment.
Requesting changes at 6018851. This was a source review plus the hosted checks on this exact head; no local tests were run.
The change itself is sound: setting app.run_inline = true makes the two tests in tests/commands.rs read and prepare the file on the frame thread, as the OCR, action, folder and reload tests already do. But the PR exists to make CI green, and on this head test (ubuntu-latest) and test (windows-latest) are both still red. The failures are in tests/document_links.rs, not in the two tests fixed here:
- ubuntu: a_remote_destination_survives_password_retries_and_cancel_opens_no_tab, an_approved_absolute_gotor_preserves_password_retries_and_cancellation
- windows: those two plus a_local_gotor_link_opens_in_a_tab_and_applies_its_destination, an_absolute_gotor_waits_for_consent_and_keeps_its_destination_across_tabs, consenting_to_absolute_gotor_still_validates_the_target
document_links.rs on main has no run_inline at all, and these tests open files and assert after a fixed number of frames, which is the same race the description names. The description also admits that other open-file tests (it names a_local_gotor_link_opens_in_a_tab_and_applies_its_destination) fail on the author's machine and were left alone.
Please fix those tests the same way, or show that they fail for a different reason. As it stands the PR does not turn main green.
|
Thanks for the review, @FredApps. Closing this as redundant: |
What this changes
CI is red on
mainand on every open PR because of two tests that race the open worker:a_tool_chosen_with_no_document_asks_for_one_and_runs_on_itandthe_open_recent_menu_lists_files_and_opens_one(crates/ui-egui/tests/commands.rs). Both open a file and assert after a fixed number of frames. Since #1493 a running app reads and prepares a file on a worker thread that finishes at its own pace (open_path: the inline path is only taken before the first frame or withrun_inline), so the assertion can run first.Evidence it is
main, not a PR: the latestmainCI run (38360385134) fails exactly these two tests (Ubuntu and Windows), and so do PRs that don't touch the open path (FreeBSD too).Both tests now set
app.run_inline = true, as the OCR, action, folder and reload tests already do, so the file is read and prepared on the frame thread and the existing steps are enough. Test-only: no product code changes.How you verified it
Before the change, on an unmodified
maincheckout:cargo test -p pdfcraft-ui-egui --test commandsfailed 3 of 3 runs ona_tool_chosen_with_no_document_asks_for_one_and_runs_on_it, and 2 of 3 onthe_open_recent_menu_lists_files_and_opens_one("the recent file opened in a new tab", left 1, right 2).After: 5 of 5 runs pass (22 tests each).
cargo fmt --checkcargo clippy -p pdfcraft-ui-egui -j 2 --all-targets -- -D warningscargo test -p pdfcraft-ui-egui -j 2 --test commandscargo xtask layersandcargo xtask assetsI did not run the full workspace tests; CI does.
Not covered. On a development Mac, about a dozen other open-file tests also fail here that CI does not report (for example
a_local_gotor_link_opens_in_a_tab_and_applies_its_destination,dropping_a_pdf_on_the_window_opens_it,password_prompt_opens_and_security_tab_reports_the_details). I have not looked at their cause (it may be macOS specific, or the same race on a slower machine), so this PR leaves them alone.Never-crash
unwrap/expect/panic!/unreachable!/todo!outside tests (test-only change)unsafe🤖 Generated with Claude Code