From 6beed43fe394304d68aa1dc2b5f9a8c24dc6f2ca Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Mon, 7 Feb 2022 18:27:57 -0500 Subject: [PATCH] Address review comments - Switch to single-threaded test and fix thread-local hashmap issue. - Use apt loop instead of `loop {}` , and wait for vblank. - Remove extraneous pthread stub. --- ctru-rs/src/lib.rs | 2 +- ctru-rs/src/services/ps.rs | 51 +++++++++++++++++++++++--------------- ctru-rs/src/test_runner.rs | 33 +++++++++--------------- 3 files changed, 44 insertions(+), 42 deletions(-) diff --git a/ctru-rs/src/lib.rs b/ctru-rs/src/lib.rs index 09bd316..c66ff31 100644 --- a/ctru-rs/src/lib.rs +++ b/ctru-rs/src/lib.rs @@ -2,7 +2,7 @@ #![crate_name = "ctru"] #![feature(test)] #![feature(custom_test_frameworks)] -#![test_runner(test_runner::test_runner)] +#![test_runner(test_runner::run)] /// Call this somewhere to force Rust to link some required crates /// This is also a setup for some crate integration only available at runtime diff --git a/ctru-rs/src/services/ps.rs b/ctru-rs/src/services/ps.rs index 9a664e8..b6f1702 100644 --- a/ctru-rs/src/services/ps.rs +++ b/ctru-rs/src/services/ps.rs @@ -94,37 +94,48 @@ mod tests { fn construct_hash_map() { let _ps = Ps::init().unwrap(); - let mut m: HashMap = HashMap::from_iter([ + let mut input = vec![ (1_i32, String::from("123")), (2, String::from("2")), (6, String::from("six")), - ]); + ]; - println!("{:?}", m); + let map: HashMap = HashMap::from_iter(input.clone()); - m.remove(&2); - m.insert(5, "ok".into()); + let mut actual: Vec<_> = map.into_iter().collect(); + input.sort(); + actual.sort(); - println!("{:#?}", m); + assert_eq!(input, actual); } #[test] - #[should_panic] fn construct_hash_map_no_rand() { // Without initializing PS, we can't use `libc::getrandom` and constructing // a HashMap panics at runtime. - - let mut m: HashMap = HashMap::from_iter([ - (1_i32, String::from("123")), - (2, String::from("2")), - (6, String::from("six")), - ]); - - println!("{:?}", m); - - m.remove(&2); - m.insert(5, "ok".into()); - - println!("{:#?}", m); + // + // If any test case successfully creates a HashMap before this test, + // the thread-local RandomState in std will be initialized. We spawn + // a new thread to actually create the hash map, since even in multi-threaded + // test environment there's a chance this test wouldn't panic because + // some other test case ran before it. + // + // One downside of this approach is that the panic handler for the panicking + // thread prints to the console, which is not captured by the default test + // harness and prints even when the test passes. + crate::thread::Builder::new() + .stack_size(0x20_0000) + .spawn(|| { + let map: HashMap = HashMap::from_iter([ + (1_i32, String::from("123")), + (2, String::from("2")), + (6, String::from("six")), + ]); + + dbg!(map); + }) + .unwrap() + .join() + .expect_err("should have panicked"); } } diff --git a/ctru-rs/src/test_runner.rs b/ctru-rs/src/test_runner.rs index f400d32..6b59651 100644 --- a/ctru-rs/src/test_runner.rs +++ b/ctru-rs/src/test_runner.rs @@ -9,15 +9,17 @@ use test::{ColorConfig, Options, OutputFormat, RunIgnored, TestDescAndFn, TestFn use crate::console::Console; use crate::gfx::Gfx; use crate::services::hid::{Hid, KeyPad}; +use crate::services::Apt; /// A custom runner to be used with `#[test_runner]`. This simple implementation /// runs all tests in series, "failing" on the first one to panic (really, the /// panic is just treated the same as any normal application panic). -pub(crate) fn test_runner(tests: &[&TestDescAndFn]) { +pub(crate) fn run(tests: &[&TestDescAndFn]) { crate::init(); let gfx = Gfx::default(); let hid = Hid::init().unwrap(); + let apt = Apt::init().unwrap(); let mut top_screen = gfx.top_screen.borrow_mut(); top_screen.set_wide_mode(true); @@ -38,18 +40,17 @@ pub(crate) fn test_runner(tests: &[&TestDescAndFn]) { exclude_should_panic: false, run_ignored: RunIgnored::No, run_tests: true, - // Benchmarks are not supported + // Don't run benchmarks. We may want to create a separate runner for them in the future bench_benchmarks: false, logfile: None, nocapture: false, // TODO: color doesn't work because of TERM/TERMINFO. // With RomFS we might be able to fake this out nicely... - color: ColorConfig::AlwaysColor, + color: ColorConfig::AutoColor, format: OutputFormat::Pretty, shuffle: false, shuffle_seed: None, - // tweak values? This seems to work out of the box - test_threads: Some(3), + test_threads: None, skip: Vec::new(), time_options: None, options: Options::new(), @@ -61,19 +62,19 @@ pub(crate) fn test_runner(tests: &[&TestDescAndFn]) { // Make sure the user can actually see the results before we exit println!("Press START to exit."); - gfx.flush_buffers(); - gfx.swap_buffers(); + while apt.main_loop() { + gfx.flush_buffers(); + gfx.swap_buffers(); + gfx.wait_for_vblank(); - loop { hid.scan_input(); - if hid.keys_down().contains(KeyPad::KEY_START) { break; } } } -/// Adapted from [`test::test_main_static`], along with [`make_owned_test`]. +/// Adapted from [`test::test_main_static`] and [`test::make_owned_test`]. fn run_static_tests(opts: &TestOpts, tests: &[&TestDescAndFn]) -> io::Result { let tests = tests.iter().map(make_owned_test).collect(); test::run_tests_console(opts, tests) @@ -98,8 +99,7 @@ fn make_owned_test(test: &&TestDescAndFn) -> TestDescAndFn { } /// The following functions are stubs needed to link the test library, -/// but do nothing because we don't actually need them for it to work (hopefully). -// TODO: move to linker-fix-3ds ? +/// but do nothing because we don't actually need them for the runner to work. mod link_fix { #[no_mangle] extern "C" fn execvp( @@ -114,15 +114,6 @@ mod link_fix { -1 } - #[no_mangle] - extern "C" fn pthread_sigmask( - _how: ::libc::c_int, - _set: *const libc::sigset_t, - _oldset: *mut libc::sigset_t, - ) -> ::libc::c_int { - -1 - } - #[no_mangle] extern "C" fn sigemptyset(_arg1: *mut libc::sigset_t) -> ::libc::c_int { -1