Test: add an AsyncRuntime test suite

[SteveLauC]

What does this PR do

Implements the test suite for AsyncRuntime and tests the built-in TokioRuntime impl with it.

Checklist

• Updated guide with pertinent info (may not always apply).
  I think we need to mention this in getting-started.md but I am not sure about where to add it.
• Squash down commits to one or two logical commits which clearly describe the work you've done.
• Unittest is a friend:)

---

This change is 

[SteveLauC]

Well, I will handle those CI failures, they are easy to deal with, I simply need to correct the import path.

[drmingdrmer]

Wow. That's a plenty of work!

Reviewed 38 of 40 files at r1, all commit messages.
Reviewable status: 38 of 40 files reviewed, 1 unresolved discussion (waiting on @schreter and @SteveLauC)

---

openraft/src/testing/mod.rs line 40 at r1 (raw file):

        crate::Membership::new(config, None),
    )
}

These functions are intended for testing purposes only, yet they remain public. They are utilized not only for log tests but also for state machine and network tests. Organizing them into a separate directory is feasible, but we should strive to minimize disruption to user applications by maintaining the existing paths to these functions wherever possible.

Code quote:

mod store_builder;
mod suite;

use std::collections::BTreeSet;

pub use store_builder::StoreBuilder;
pub use suite::Suite;

use crate::entry::RaftEntry;
use crate::CommittedLeaderId;
use crate::LogId;
use crate::RaftTypeConfig;

/// Builds a log id, for testing purposes.
pub fn log_id<NID: crate::NodeId>(term: u64, node_id: NID, index: u64) -> LogId<NID> {
    LogId::<NID> {
        leader_id: CommittedLeaderId::new(term, node_id),
        index,
    }
}

/// Create a blank log entry for test.
pub fn blank_ent<C: RaftTypeConfig>(term: u64, node_id: C::NodeId, index: u64) -> crate::Entry<C> {
    crate::Entry::<C>::new_blank(LogId::new(CommittedLeaderId::new(term, node_id), index))
}

/// Create a membership log entry without learner config for test.
pub fn membership_ent<C: RaftTypeConfig>(
    term: u64,
    node_id: C::NodeId,
    index: u64,
    config: Vec<BTreeSet<C::NodeId>>,
) -> crate::Entry<C> {
    crate::Entry::new_membership(
        LogId::new(CommittedLeaderId::new(term, node_id), index),
        crate::Membership::new(config, None),
    )
}

[SteveLauC]

but we should strive to minimize disruption to user applications by maintaining the existing paths to these functions wherever possible.

Ah, right, I forgot they're public APIs, will try to maintain their current import path.

[SteveLauC]

Reviewable status: 3 of 47 files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @schreter)

---

openraft/src/testing/mod.rs line 40 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

These functions are intended for testing purposes only, yet they remain public. They are utilized not only for log tests but also for state machine and network tests. Organizing them into a separate directory is feasible, but we should strive to minimize disruption to user applications by maintaining the existing paths to these functions wherever possible.

I moved them to a new file common.rs and re-exported them in log/mod.rs so that their import paths remain the same:)

[SteveLauC]

Reviewable status: 3 of 47 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/testing/runtime/mod.rs line 36 at r3 (raw file):

impl<Rt: AsyncRuntime> Suite<Rt> {
    pub async fn test_all() {

I purposely make this an async function so that we don't need to create users' runtime and block_on() the tests on the Openraft side, and this is currently impossible as there is no such block_on() function in the AsyncRuntime trait.

struct MyCustomRuntime;
impl openraft::AsyncRuntime for MyCustomRuntime { /* omitted */ }

// Build a runtime
let rt = MyCustomRuntime::new();
// Run all the tests
rt.block_on(Suite::<MyCustomRuntime>::test_all());

BTW, currently, our log test suite hardcodes the runtime to Tokio, I think we need to make it runtime-agnostic as well

[drmingdrmer]

Reviewed 1 of 40 files at r1, 7 of 7 files at r2, 43 of 43 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter and @SteveLauC)

---

openraft/src/testing/mod.rs line 40 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

I moved them to a new file common.rs and re-exported them in log/mod.rs so that their import paths remain the same:)

Good!

---

openraft/src/testing/runtime/mod.rs line 36 at r3 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

I purposely make this an async function so that we don't need to create users' runtime and block_on() the tests on the Openraft side, and this is currently impossible as there is no such block_on() function in the AsyncRuntime trait.

struct MyCustomRuntime;
impl openraft::AsyncRuntime for MyCustomRuntime { /* omitted */ }

// Build a runtime
let rt = MyCustomRuntime::new();
// Run all the tests
rt.block_on(Suite::<MyCustomRuntime>::test_all());

BTW, currently, our log test suite hardcodes the runtime to Tokio, I think we need to make it runtime-agnostic as well

Make sense.

[SteveLauC]

@schreter, hi, would like to have your review as well, could you please take a look:)

[schreter]

@schreter, hi, would like to have your review as well, could you please take a look:)

I didn't forget you, but I have too much on my plate :-(.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SteveLauC)

[schreter]

Just a quick review with some comments.

Reviewed 3 of 40 files at r1, 1 of 7 files at r2, 43 of 43 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @SteveLauC)

---

openraft/src/testing/runtime/mod.rs line 67 at r3 (raw file):

    pub async fn test_sleep() {
        let start_time = std::time::Instant::now();
        let dur_100ms = std::time::Duration::from_millis(100);

I'd suggest to lower this to 10ms or so

same below (several times)

Also, you can factor this out to a constant to reuse in multiple tests.

---

openraft/src/testing/runtime/mod.rs line 71 at r3 (raw file):

        let elapsed = start_time.elapsed();

        assert!(elapsed > dur_100ms);

>= to be on the safe side (very unlikely it'll hit)

same below

---

openraft/src/testing/runtime/mod.rs line 101 at r3 (raw file):

        // Will time out
        let dur_150ms = std::time::Duration::from_millis(150);

I'd suggest to increase this to a second or more, since you can have a hiccup on the system blocking it for couple hundred ms (even for seconds, but unlikely).

Same below.

---

openraft/src/testing/runtime/mod.rs line 180 at r3 (raw file):

        for idx in 0..n_senders {
            let tx = Arc::clone(&tx);

Why not simply tx.clone()?

---

openraft/src/testing/runtime/mod.rs line 227 at r3 (raw file):

        tx.send(overwrite).unwrap();
        rx.changed().await.unwrap();

It would be good to take this future also before send() to verify that it indeed changes state afterwards (in a second test/next step of the test). For that, you can do something like this:

let fut = rx.changed();
let fut = Pin::new(&mut fut);
assert!(poll_in_place(fut), matches!(Poll::Pending));
tx.send(...);
assert!(poll_in_place(fut), matches!(Poll::Ready(...)));

There is some support for such feats in futures crate. For example, you can do this:

pub fn poll_in_place<F: Future>(fut: Pin<&mut F>) -> Poll<F::Output> {
    let waker = futures::task::noop_waker();
    let mut cx = Context::from_waker(&waker);
    fut.poll(&mut cx)
}

You can also use it elsewhere to fine-grained check when the future becomes ready, especially for sync primitives.

---

openraft/src/testing/runtime/mod.rs line 316 at r3 (raw file):

            handles.push(handle);
        }

This is most likely unreliable for a negative test. You don't know where and when the tasks get spawned. They may as well execute sequentially and you won't see broken mutex.

However, you can create a second (or more) futures from mutex.lock() and simply check they are not ready and become ready immediately after releasing the lock using poll_in_place() mentioned above.

Code quote:

        for _ in 0..n_task {
            let counter = Arc::clone(&counter);
            let handle = Rt::spawn(async move {
                let mut guard = counter.lock().await;
                *guard += 1;
            });

            handles.push(handle);
        }

[SteveLauC]

Reviewable status: 46 of 47 files reviewed, 4 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/testing/runtime/mod.rs line 67 at r3 (raw file):

Previously, schreter wrote…

I'd suggest to lower this to 10ms or so

same below (several times)

Also, you can factor this out to a constant to reuse in multiple tests.

done

---

openraft/src/testing/runtime/mod.rs line 71 at r3 (raw file):

Previously, schreter wrote…

>= to be on the safe side (very unlikely it'll hit)

same below

done

---

openraft/src/testing/runtime/mod.rs line 101 at r3 (raw file):

Previously, schreter wrote…

I'd suggest to increase this to a second or more, since you can have a hiccup on the system blocking it for couple hundred ms (even for seconds, but unlikely).

Same below.

done

---

openraft/src/testing/runtime/mod.rs line 180 at r3 (raw file):

Previously, schreter wrote…

Why not simply tx.clone()?

Arc::clone(&p) is generally considered a better style than p.clone() when it comes to reference-counted pointers, there is a lint for it (disabled by default).

Changed to tx.clone() as Openraft does not use this style.

[SteveLauC]

openraft/src/testing/runtime/mod.rs line 227 at r3 (raw file):

Previously, schreter wrote…

It would be good to take this future also before send() to verify that it indeed changes state afterwards (in a second test/next step of the test). For that, you can do something like this:

let fut = rx.changed();
let fut = Pin::new(&mut fut);
assert!(poll_in_place(fut), matches!(Poll::Pending));
tx.send(...);
assert!(poll_in_place(fut), matches!(Poll::Ready(...)));

There is some support for such feats in futures crate. For example, you can do this:

pub fn poll_in_place<F: Future>(fut: Pin<&mut F>) -> Poll<F::Output> {
    let waker = futures::task::noop_waker();
    let mut cx = Context::from_waker(&waker);
    fut.poll(&mut cx)
}

You can also use it elsewhere to fine-grained check when the future becomes ready, especially for sync primitives.

done, see this commit: 46ffc3e

[SteveLauC]

Reviewable status: 46 of 47 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/testing/runtime/mod.rs line 316 at r3 (raw file):
I renamed this test to test_mutex_contention() and added another test to test the case you described:

However, you can create a second (or more) futures from mutex.lock() and simply check they are not ready and become ready immediately after releasing the lock using poll_in_place() mentioned above.

See this commit 77ad0ab

[schreter]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @SteveLauC)

---

openraft/src/testing/runtime/mod.rs line 180 at r3 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Arc::clone(&p) is generally considered a better style than p.clone() when it comes to reference-counted pointers, there is a lint for it (disabled by default).

Changed to tx.clone() as Openraft does not use this style.

Interesting opinion, which certainly has some merits.

According to an old discussion here, it's not that popular, though...

The standard docs here say "Arc<T>’s implementations of traits like Clone may also be called using fully qualified syntax. Some people prefer to use fully qualified syntax, while others prefer using method-call syntax." (highlight mine). Since quite a few examples in Arc use fully-qualified syntax, I'd say there is no agreement among library writers here :-).

Anyway, since the whole codebase uses clone() method call directly, I'd also go with clone() method call here.

---

openraft/src/testing/runtime/mod.rs line 362 at r6 (raw file):

}

/// A helper function to peek a future's state.

It actually polls, not peeks :-).

[SteveLauC]

Reviewable status: 46 of 47 files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @schreter)

---

openraft/src/testing/runtime/mod.rs line 362 at r6 (raw file):

Previously, schreter wrote…

It actually polls, not peeks :-).

Right, updated:)

[schreter]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer)

[drmingdrmer]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SteveLauC)

[SteveLauC]

Commits squashed.

[SteveLauC]

Let me rebase my branch to handle the conflicts:)