Skip to content

Commit

Permalink
chore(maitake): actually run loom tests on CI (#483)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hawkw committed Aug 3, 2024
1 parent 45cf523 commit 2b475ab
Show file tree
Hide file tree
Showing 12 changed files with 99 additions and 23 deletions.
2 changes: 1 addition & 1 deletion .config/nextest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
final-status-level = "skip"
43 changes: 41 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -301,4 +320,24 @@ jobs:
- name: install nextest
uses: taiki-e/install-action@nextest
- name: run loom tests
run: just loom mycelium-util
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
2 changes: 1 addition & 1 deletion justfile
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}

Expand Down
12 changes: 7 additions & 5 deletions maitake/src/scheduler/tests/loom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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...
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion maitake/src/task/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:#?}");
Expand Down
2 changes: 2 additions & 0 deletions maitake/src/task/tests/alloc_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 8 additions & 0 deletions maitake/src/task/tests/loom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(());
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 4 additions & 0 deletions maitake/src/time/timer/tests/wheel_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,10 @@ fn fuzz_action_strategy() -> impl Strategy<Value = FuzzAction> {

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());
Expand Down
15 changes: 15 additions & 0 deletions maitake/src/time/timer/wheel/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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));
Expand All @@ -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}");
Expand Down
6 changes: 2 additions & 4 deletions maitake/tests/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down Expand Up @@ -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));

Expand Down
15 changes: 9 additions & 6 deletions maitake/tests/scheduler/alloc.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::*;
use mycelium_util::sync::{Lazy, spin::Mutex};
use mycelium_util::sync::{spin::Mutex, Lazy};

#[test]
fn basically_works() {
Expand All @@ -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<StaticScheduler> = Lazy::new(StaticScheduler::new);
static COMPLETED: AtomicUsize = AtomicUsize::new(0);

const TASKS: usize = 10;

util::trace_init();
for _ in 0..TASKS {
SCHEDULER.spawn(async {
Expand All @@ -49,8 +51,6 @@ fn many_yields() {
static SCHEDULER: Lazy<StaticScheduler> = Lazy::new(StaticScheduler::new);
static COMPLETED: AtomicUsize = AtomicUsize::new(0);

const TASKS: usize = 10;

util::trace_init();

for i in 0..TASKS {
Expand Down Expand Up @@ -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();
Expand Down
7 changes: 4 additions & 3 deletions maitake/tests/scheduler/custom_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down

0 comments on commit 2b475ab

Please sign in to comment.