Conversation
908769c to
fd2358b
Compare
|
I think this is a neat idea and makes sense. Would be good though if either Leon or Olivier could review this approach - I think they may have more of an opinion on how to achieve this :) |
|
Does that mean we should remove the |
|
Yes, for Two things to decide before dropping it:
I can do the CI cleanup in this PR or as a follow-up, whichever you prefer. |
Yeah, i think it won't make a difference since they are built either way and running them doesn't take any time.
indeed, maybe the interpreter test needs a similar trick |
fd2358b to
ea5410e
Compare
|
Done in ea5410e, on top of a rebase onto master: the One correction to my previous comment: the integration step used |
This would still need to reach into Qt for things like default size and stuff like that which may cause problems as they still use non-trhead-safe APIs |
LeonMatthes
left a comment
There was a problem hiding this comment.
The solution itself is fine, but we already have a better way to do this statically under tests/backends/cases/harness.rs which requires less code generation at build time.
| fn write_main_thread_harness( | ||
| output: &mut dyn Write, | ||
| tests: &[MainThreadTest], | ||
| ) -> std::io::Result<()> { |
There was a problem hiding this comment.
Instead of generating a full harness from text every time, can we just ship the harness as a file under driver/rust/tests/widgets-qt that just uses include! to collect the tests?
(or use satchel for test collection).
That makes this a normal editable Rust file again, instead of generating it at build time.
Note: We have exactly this setup under /tests/backends/ for the qt and winit tests which must also run on the main thread with custom init code. There the code lives under cases/harness.rs.
Ideally we should just reuse that approach and ideally the code.
Note: if we use satchel, we might not even have to adjust the generated code for the tests, as we can keep using #[test] for test collection and we just have to enable our own harness.
There was a problem hiding this comment.
Done in 8366347. The harness is now a normal file, and it's the one from tests/backends: cases/harness.rs moved into test_driver_lib::fork_harness behind a fork-harness feature, and both the backends targets and widgets-qt call it from there. The generated qt modules use #[satchel::test], so build.rs only swaps the attribute and wraps the Result body; tests/widgets-qt.rs is hand-written and calls test_main(satchel::get_tests!(), ...).
Measured locally on macOS: widgets-qt 29/29 on both the macro and build-time paths, test-backends qt and winit 11/11 each through the shared code, --list, --skip=qt and a cross-target --exact behave as before.
ea5410e to
8366347
Compare
|
Two updates:
The macOS |
On macOS the qt style creates AppKit controls, and AppKit deadlocks on any thread but the process main thread. libtest runs every test on a worker thread, so the widgets-qt driver tests hung forever in NativeSlider's QMacStyle hit test. Give the widgets-qt target its own harness, following the pattern of the test-backends crate: libtest-mimic forks one subprocess per test, and each subprocess runs its single test on its own main thread with its own thread-local testing platform. CI's '--skip=qt::' filter and '--list' behave as before.
The widgets-qt target no longer generates its harness at build time. The generated qt modules register their tests with #[satchel::test], and a hand-written tests/widgets-qt.rs runs them through test_driver_lib::fork_harness, which is the harness from tests/backends/tests/cases/harness.rs moved into the driver library behind a fork-harness feature. The backends crate's qt and winit targets use it from there. The shared harness also honors #[ignore] on the forked trials.
8366347 to
6917272
Compare
| r" | ||
| #[rust_analyzer::skip] | ||
| #[satchel::test] {} fn t_{}() {{ | ||
| (|| -> ::std::result::Result<(), ::std::boxed::Box<dyn ::std::error::Error>> {{ |
There was a problem hiding this comment.
Just curious: why do we use a closure here?
There was a problem hiding this comment.
The snippets use ?, so they need a body that returns Result, as in the #[test] branch below it.
satchel registers tests as fn() (TestFn in satchel 0.3) and doesn't wrap other return types the way libtest does.
So the closure gives the snippet a Result-returning scope, and .unwrap() turns an Err into the panic satchel counts as a failure.
I can make it a named inner fn body() -> Result<…> plus body().unwrap() if that reads better. It does the same thing.
On macOS,
cargo test -p test-driver-rust --test widgets-qthangs forever with 0% CPU. The qt style calls QStyle from the thread running the test, and QMacStyle instantiates AppKit controls for hit testing. On current macOS, AppKit's control layout goes through SwiftUI and blocks until it can run on the process main thread. libtest runs every test on a worker thread while the main thread waits for results, so the firstNativeSliderhit test deadlocks:--test-threads=1doesn't help: libtest still spawns a worker thread per test. CI never saw the hang because it kept the qt tests out of the regular steps with--skipfilters and ran them in a separate pass with--test-threads=1.This complements #13370, which runs
tests/run_tests.shoffscreen for machines without a display. With a display, the offscreen platform is not in effect for plaincargo test, and this change also lets the tests exercise the real macOS Qt style.Design
The
widgets-qttarget getsharness = false. Its generated modules register their tests with#[satchel::test], and the hand-writtentests/widgets-qt.rsruns them throughtest_driver_lib::fork_harness: the parent forks one subprocess per test (marked with--exact <name>, the same convention cargo nextest uses), and each subprocess runs its single test on its own main thread with its own thread-local testing platform. Parallelism is preserved because every subprocess has its own main thread.The harness is the one from
tests/backends/tests/cases/harness.rs, moved into the driver library behind afork-harnessfeature; the backends crate's qt and winit targets use it from there. It also honors#[ignore]on the forked trials.The libtest CLI surface stays compatible:
--skip=qtfilters all tests out as before,--listprints the same names, and an--exactfilter that names a test from another target reports zero matching tests instead of failing.Testing
Everything measured on macOS arm64 (macOS 26.6, Qt with QMacStyle):
widgets-qt: 25/25 pass in ~7s, on both the macro and thebuild-timegeneration paths. Both previously hung on the first slider hit test.test-driver-rustsuite: 35/35 test binaries, 855 passed, 0 failed. The suite could not complete on this machine before.-- --skip=qt::(the CI invocation),-- --list, and cross-target-- --exactselection verified against the built binary.CI
Unchanged. The serialized Qt pass still covers the interpreter driver's
_qttests, which reach into Qt for size hints from worker threads, and thetests/backendsqt target, which the--skip=qtfilter keeps off Windows. Moving those onto the fork harness and dropping the pass is a follow-up.Follow-up
Run the interpreter driver's qt tests and the
tests/backendsqt target through the fork harness too, then drop the serialized Qt pass and the--skip=qtfilters from CI.