From 2b475ab858fb1e55d0538e060abba3564f8cb3c9 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 3 Aug 2024 11:33:54 -0700 Subject: [PATCH] chore(maitake): actually run loom tests on CI (#483) It turns out CI only ever runs Miri tests for `cordyceps`, not `maitake`/`maitake-sync`. MY BAD LOL. This PR fixes that. In doing so, I've also had to go through and change a bunch of tests to spawn fewer tasks when running under Miri, so that it doesn't take a really long time to run. Most of the issues we hope Miri catches don't actually depend on concurrency --- we have `loom` tests for that. --- .config/nextest.toml | 2 +- .github/workflows/ci.yml | 43 ++++++++++++++++++++- justfile | 2 +- maitake/src/scheduler/tests/loom.rs | 12 +++--- maitake/src/task/state.rs | 6 ++- maitake/src/task/tests/alloc_tests.rs | 2 + maitake/src/task/tests/loom.rs | 8 ++++ maitake/src/time/timer/tests/wheel_tests.rs | 4 ++ maitake/src/time/timer/wheel/tests.rs | 15 +++++++ maitake/tests/mutex.rs | 6 +-- maitake/tests/scheduler/alloc.rs | 15 ++++--- maitake/tests/scheduler/custom_storage.rs | 7 ++-- 12 files changed, 99 insertions(+), 23 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index 54317428..6502aa88 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -27,4 +27,4 @@ final-status-level = "skip" slow-timeout = { period = "10m" } # Do not cancel the test run on the first failure. fail-fast = false -final-status-level = "skip" \ No newline at end of file +final-status-level = "skip" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4edd0ca4..334fb16a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -230,7 +230,7 @@ jobs: needs: changed_paths if: needs.changed_paths.outputs.should_skip != 'true' || !fromJSON(needs.changed_paths.outputs.paths_result).cordyceps.should_skip runs-on: ubuntu-latest - name: Miri tests (codyceps) + name: Miri tests (cordyceps) steps: - name: install rust toolchain run: rustup show @@ -282,6 +282,25 @@ jobs: - name: run loom tests (maitake-sync) run: just loom maitake-sync + # run miri tests + maitake_miri: + needs: changed_paths + if: needs.changed_paths.outputs.should_skip != 'true' || !fromJSON(needs.changed_paths.outputs.paths_result).maitake.should_skip + runs-on: ubuntu-latest + name: Miri tests (maitake) + steps: + - name: install rust toolchain + run: rustup show + - uses: actions/checkout@v4 + - name: install Just + uses: extractions/setup-just@v1 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: install nextest + uses: taiki-e/install-action@nextest + - name: run Miri tests + run: just miri maitake + ### mycelium-util ### # run loom tests @@ -301,4 +320,24 @@ jobs: - name: install nextest uses: taiki-e/install-action@nextest - name: run loom tests - run: just loom mycelium-util \ No newline at end of file + run: just loom mycelium-util + + # dummy job that depends on all required checks + all_systems_go: + needs: + - check-host + - rustfmt + - clippy + - test-host + - build-x64 + - test-x64 + - docs + - cordyceps_loom + - cordyceps_miri + - maitake_no_default_features + - maitake_loom + - maitake_miri + - util_loom + runs-on: ubuntu-latest + steps: + - run: exit 0 diff --git a/justfile b/justfile index 19d3356c..d02f96ce 100755 --- a/justfile +++ b/justfile @@ -101,7 +101,7 @@ miri crate='' *args='': _get-nextest PROPTEST_CASES="{{ env_var_or_default("PROPTEST_CASES", "10") }}" \ {{ _cargo }} miri {{ _testcmd }} \ {{ if crate == '' { miri-crates } else { '-p' } }} {{ crate }} \ - {{ _test-profile }} \ + {{ _loom-profile }} \ --lib \ {{ args }} diff --git a/maitake/src/scheduler/tests/loom.rs b/maitake/src/scheduler/tests/loom.rs index fe137763..6cc8b588 100644 --- a/maitake/src/scheduler/tests/loom.rs +++ b/maitake/src/scheduler/tests/loom.rs @@ -89,7 +89,8 @@ fn notify_future() { #[test] fn schedule_many() { - const TASKS: usize = 10; + // don't spawn as many tasks under miri, so that this test doesn't take forever... + const TASKS: usize = if cfg!(miri) { 2 } else { 10 }; // for some reason this branches slightly too many times for the default max // branches, IDK why... @@ -156,7 +157,8 @@ fn current_task() { // this hits what i *believe* is a loom bug: https://github.com/tokio-rs/loom/issues/260 #[cfg_attr(loom, ignore)] fn cross_thread_spawn() { - const TASKS: usize = 10; + // don't spawn as many tasks under miri, so that this test doesn't take forever... + const TASKS: usize = if cfg!(miri) { 2 } else { 10 }; loom::model(|| { let scheduler = Scheduler::new(); let completed = Arc::new(AtomicUsize::new(0)); @@ -198,10 +200,10 @@ fn cross_thread_spawn() { // much tracing data across a huge number of iterations. skip it for now. #[cfg_attr(loom, ignore)] fn injector() { - // when running in loom, don't spawn all ten tasks, because that makes this + // when running in loom/miri, don't spawn all ten tasks, because that makes this // test run F O R E V E R - const TASKS: usize = if cfg!(loom) { 2 } else { 10 }; - const THREADS: usize = if cfg!(loom) { 2 } else { 5 }; + const TASKS: usize = if cfg!(loom) || cfg!(miri) { 2 } else { 10 }; + const THREADS: usize = if cfg!(loom) || cfg!(miri) { 2 } else { 5 }; let _trace = crate::util::trace_init(); // for some reason this branches slightly too many times for the default max diff --git a/maitake/src/task/state.rs b/maitake/src/task/state.rs index dce2df4e..c4fac972 100644 --- a/maitake/src/task/state.rs +++ b/maitake/src/task/state.rs @@ -585,11 +585,15 @@ mod tests { use super::*; #[test] + // No sense spending time running these trivial tests under Miri... + #[cfg_attr(miri, ignore)] fn packing_specs_valid() { State::assert_valid() } - + #[test] + // No sense spending time running these trivial tests under Miri... + #[cfg_attr(miri, ignore)] fn debug_alt() { let state = StateCell::new(); println!("{state:#?}"); diff --git a/maitake/src/task/tests/alloc_tests.rs b/maitake/src/task/tests/alloc_tests.rs index 0b1c0686..d01447e7 100644 --- a/maitake/src/task/tests/alloc_tests.rs +++ b/maitake/src/task/tests/alloc_tests.rs @@ -39,6 +39,8 @@ fn task_is_valid_for_casts() { /// This test just prints the size (in bytes) of an empty task struct. #[test] +// No sense spending time running these trivial tests under Miri... +#[cfg_attr(miri, ignore)] fn empty_task_size() { use core::{ any::type_name, diff --git a/maitake/src/task/tests/loom.rs b/maitake/src/task/tests/loom.rs index a182ca9b..55d60f6c 100644 --- a/maitake/src/task/tests/loom.rs +++ b/maitake/src/task/tests/loom.rs @@ -29,6 +29,9 @@ fn taskref_deallocates() { #[test] #[should_panic] +// Miri (correctly) detects a memory leak in this test and fails, which is +// good...but Miri doesn't understand "should panic". +#[cfg_attr(miri, ignore)] fn do_leaks_work() { loom::model(|| { let track = Track::new(()); @@ -80,6 +83,11 @@ fn joinhandle_deallocates() { } #[test] +// The non-loom version of ths test uses a Tokio API that calls into +// `epoll_wait`, which Miri doesn't simulate currently. Thus, ignore it. The +// version in `alloc_tests` should simulate the same behavior under Miri anyway, +// so we don't need to run this. +#[cfg_attr(miri, ignore)] fn join_handle_wakes() { loom::model(|| { let scheduler = Scheduler::new(); diff --git a/maitake/src/time/timer/tests/wheel_tests.rs b/maitake/src/time/timer/tests/wheel_tests.rs index 2ea66f53..a676ea41 100644 --- a/maitake/src/time/timer/tests/wheel_tests.rs +++ b/maitake/src/time/timer/tests/wheel_tests.rs @@ -424,6 +424,10 @@ fn fuzz_action_strategy() -> impl Strategy { proptest! { #[test] + // This test intentionally leaks the timer into a static, which is detected + // by Miri's leak checking. Eventually we should figure out a way to make it + // work without Miri getting mad... + #[cfg_attr(miri, ignore)] fn fuzz_timer(actions in vec(fuzz_action_strategy(), 0..MAX_FUZZ_ACTIONS)) { static FUZZ_RUNS: AtomicUsize = AtomicUsize::new(1); static TIMER: Timer = Timer::new(TestClock::clock()); diff --git a/maitake/src/time/timer/wheel/tests.rs b/maitake/src/time/timer/wheel/tests.rs index 5c8a7176..2e6abde2 100644 --- a/maitake/src/time/timer/wheel/tests.rs +++ b/maitake/src/time/timer/wheel/tests.rs @@ -2,6 +2,9 @@ use super::*; use proptest::{prop_assert_eq, proptest}; #[test] +// This test doesn't exercise anything that's potentially memory-unsafe, so +// don't spend time running it under miri. +#[cfg_attr(miri, ignore)] fn wheel_indices() { let core = Core::new(); for ticks in 0..64 { @@ -43,11 +46,17 @@ fn wheel_indices() { } #[test] +// This test doesn't exercise anything that's potentially memory-unsafe, so +// don't spend time running it under miri. +#[cfg_attr(miri, ignore)] fn bitshift_is_correct() { assert_eq!(1 << Wheel::BITS, Wheel::SLOTS); } #[test] +// This test doesn't exercise anything that's potentially memory-unsafe, so +// don't spend time running it under miri. +#[cfg_attr(miri, ignore)] fn slot_indices() { let wheel = Wheel::new(0); for i in 0..64 { @@ -66,6 +75,9 @@ fn slot_indices() { } #[test] +// This test doesn't exercise anything that's potentially memory-unsafe, so +// don't spend time running it under miri. +#[cfg_attr(miri, ignore)] fn test_next_set_bit() { assert_eq!(dbg!(next_set_bit(0b0000_1001, 2)), Some(3)); assert_eq!(dbg!(next_set_bit(0b0000_1001, 3)), Some(3)); @@ -79,6 +91,9 @@ fn test_next_set_bit() { proptest! { #[test] + // This test doesn't exercise anything that's potentially memory-unsafe, so + // don't spend time running it under miri. + #[cfg_attr(miri, ignore)] fn next_set_bit_works(bitmap: u64, offset in 0..64u32) { println!(" bitmap: {bitmap:064b}"); println!(" offset: {offset}"); diff --git a/maitake/tests/mutex.rs b/maitake/tests/mutex.rs index a7b29e7c..f68b1992 100644 --- a/maitake/tests/mutex.rs +++ b/maitake/tests/mutex.rs @@ -22,10 +22,10 @@ mod alloc { use maitake::scheduler::Scheduler; use std::{future::Future, sync::Arc}; + const TASKS: usize = if cfg!(miri) { 2 } else { 10 }; + #[test] fn basically_works() { - const TASKS: usize = 10; - let scheduler = Scheduler::new(); let lock = Arc::new(Mutex::new(0)); @@ -53,8 +53,6 @@ mod alloc { #[test] #[cfg(feature = "alloc")] fn lock_owned() { - const TASKS: usize = 10; - let scheduler = Scheduler::new(); let lock = Arc::new(Mutex::new(0)); diff --git a/maitake/tests/scheduler/alloc.rs b/maitake/tests/scheduler/alloc.rs index 7909e817..00bdcde3 100644 --- a/maitake/tests/scheduler/alloc.rs +++ b/maitake/tests/scheduler/alloc.rs @@ -1,5 +1,5 @@ use super::*; -use mycelium_util::sync::{Lazy, spin::Mutex}; +use mycelium_util::sync::{spin::Mutex, Lazy}; #[test] fn basically_works() { @@ -21,13 +21,15 @@ fn basically_works() { assert_eq!(tick.polled, 2) } +// Don't spawn as many tasks under Miri so that the test can run in a +// reasonable-ish amount of time. +const TASKS: usize = if cfg!(miri) { 2 } else { 10 }; + #[test] fn schedule_many() { static SCHEDULER: Lazy = Lazy::new(StaticScheduler::new); static COMPLETED: AtomicUsize = AtomicUsize::new(0); - const TASKS: usize = 10; - util::trace_init(); for _ in 0..TASKS { SCHEDULER.spawn(async { @@ -49,8 +51,6 @@ fn many_yields() { static SCHEDULER: Lazy = Lazy::new(StaticScheduler::new); static COMPLETED: AtomicUsize = AtomicUsize::new(0); - const TASKS: usize = 10; - util::trace_init(); for i in 0..TASKS { @@ -100,7 +100,10 @@ fn steal_blocked() { assert!(SCHEDULER_1.current_task().is_some()); - let stolen = SCHEDULER_1.try_steal().unwrap().spawn_n(&SCHEDULER_2.get(), 1); + let stolen = SCHEDULER_1 + .try_steal() + .unwrap() + .spawn_n(&SCHEDULER_2.get(), 1); assert_eq!(stolen, 1); let tick = SCHEDULER_2.tick(); diff --git a/maitake/tests/scheduler/custom_storage.rs b/maitake/tests/scheduler/custom_storage.rs index 56e2c84a..7e1e42ab 100644 --- a/maitake/tests/scheduler/custom_storage.rs +++ b/maitake/tests/scheduler/custom_storage.rs @@ -75,14 +75,16 @@ fn static_scheduler_macro() { assert_eq!(tick.polled, 2) } +// Don't spawn as many tasks under Miri so that the test can run in a +// reasonable-ish amount of time. +const TASKS: usize = if cfg!(miri) { 2 } else { 10 }; + #[test] fn schedule_many() { static STUB: TaskStub = TaskStub::new(); static SCHEDULER: StaticScheduler = unsafe { StaticScheduler::new_with_static_stub(&STUB) }; static COMPLETED: AtomicUsize = AtomicUsize::new(0); - const TASKS: usize = 10; - util::trace_init(); for _ in 0..TASKS { @@ -107,7 +109,6 @@ fn many_yields() { static COMPLETED: AtomicUsize = AtomicUsize::new(0); util::trace_init(); - const TASKS: usize = 10; for i in 0..TASKS { MyBoxTask::spawn(&SCHEDULER, async move {