Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Fix flaky test (#6131)
Browse files Browse the repository at this point in the history
* Split test + decrease test timeout

* fmt

* spellcheck
  • Loading branch information
slumber authored Oct 10, 2022
1 parent f85f96c commit e36cc59
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 20 deletions.
4 changes: 4 additions & 0 deletions node/network/collator-protocol/src/validator_side/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,16 @@ const BENEFIT_NOTIFY_GOOD: Rep =
/// to finish on time.
///
/// There is debug logging output, so we can adjust this value based on production results.
#[cfg(not(test))]
const MAX_UNSHARED_DOWNLOAD_TIME: Duration = Duration::from_millis(400);

// How often to check all peers with activity.
#[cfg(not(test))]
const ACTIVITY_POLL: Duration = Duration::from_secs(1);

#[cfg(test)]
const MAX_UNSHARED_DOWNLOAD_TIME: Duration = Duration::from_millis(100);

#[cfg(test)]
const ACTIVITY_POLL: Duration = Duration::from_millis(10);

Expand Down
50 changes: 30 additions & 20 deletions node/network/collator-protocol/src/validator_side/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use futures::{executor, future, Future};
use sp_core::{crypto::Pair, Encode};
use sp_keyring::Sr25519Keyring;
use sp_keystore::{testing::KeyStore as TestKeyStore, SyncCryptoStore};
use std::{iter, sync::Arc, time::Duration};
use std::{iter, sync::Arc, task::Poll, time::Duration};

use polkadot_node_network_protocol::{
our_view,
Expand Down Expand Up @@ -493,17 +493,11 @@ fn collator_authentication_verification_works() {
});
}

// A test scenario that takes the following steps
// - Two collators connect, declare themselves and advertise a collation relevant to
// our view.
// - Collation protocol should request one PoV.
// - Collation protocol should disconnect both collators after having received the collation.
// - The same collators plus an additional collator connect again and send `PoV`s for a different relay parent.
// - Collation protocol will request one PoV, but we will cancel it.
// - Collation protocol should request the second PoV which does not succeed in time.
// - Collation protocol should request third PoV.
/// Tests that a validator fetches only one collation at any moment of time
/// per relay parent and ignores other advertisements once a candidate gets
/// seconded.
#[test]
fn fetch_collations_works() {
fn fetch_one_collation_at_a_time() {
let test_state = TestState::default();

test_harness(|test_harness| async move {
Expand Down Expand Up @@ -575,22 +569,38 @@ fn fetch_collations_works() {
)
.await;

overseer_send(
&mut virtual_overseer,
CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerDisconnected(
peer_b.clone(),
)),
)
.await;
// Ensure the subsystem is polled.
test_helpers::Yield::new().await;

// Second collation is not requested since there's already seconded one.
assert_matches!(futures::poll!(virtual_overseer.recv().boxed()), Poll::Pending);

virtual_overseer
})
}

/// Tests that a validator starts fetching next queued collations on [`MAX_UNSHARED_DOWNLOAD_TIME`]
/// timeout and in case of an error.
#[test]
fn fetches_next_collation() {
let test_state = TestState::default();

test_harness(|test_harness| async move {
let TestHarness { mut virtual_overseer } = test_harness;

let second = Hash::random();

overseer_send(
&mut virtual_overseer,
CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerDisconnected(
peer_c.clone(),
CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(
our_view![test_state.relay_parent, second],
)),
)
.await;

respond_to_core_info_queries(&mut virtual_overseer, &test_state).await;
respond_to_core_info_queries(&mut virtual_overseer, &test_state).await;

let peer_b = PeerId::random();
let peer_c = PeerId::random();
let peer_d = PeerId::random();
Expand Down
29 changes: 29 additions & 0 deletions node/subsystem-test-helpers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use sp_core::testing::TaskExecutor;

use std::{
convert::Infallible,
future::Future,
pin::Pin,
sync::Arc,
task::{Context, Poll, Waker},
Expand Down Expand Up @@ -391,6 +392,34 @@ macro_rules! arbitrary_order {
};
}

/// Future that yields the execution once and resolves
/// immediately after.
///
/// Useful when one wants to poll the background task to completion
/// before sending messages to it in order to avoid races.
pub struct Yield(bool);

impl Yield {
/// Returns new `Yield` future.
pub fn new() -> Self {
Self(false)
}
}

impl Future for Yield {
type Output = ();

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
if !self.0 {
self.0 = true;
cx.waker().wake_by_ref();
Poll::Pending
} else {
Poll::Ready(())
}
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down

0 comments on commit e36cc59

Please sign in to comment.