Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for connected-peers protocol (DSN). #1874

Merged
merged 4 commits into from
Aug 29, 2023
Merged

Add tests for connected-peers protocol (DSN). #1874

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc4aa61
networking: Add tests for connected-peers protocol.
shamil-gadelshin Aug 23, 2023
63994b6
networking: Update tests for connected-peers protocol.
shamil-gadelshin Aug 24, 2023
e2e7cb5
Merge branch 'main' into connected-peers-tests
shamil-gadelshin Aug 29, 2023
adb5500
networking: Fix connected-peers test.
shamil-gadelshin Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions crates/subspace-networking/src/protocols/connected_peers/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ struct ConnectedPeersInstance;

#[tokio::test()]
async fn test_connection_breaks_after_timeout_without_decision() {
let decision_timeout = Duration::from_millis(500);
let large_delay = Duration::from_millis(2000);
let decision_timeout = Duration::from_millis(300);
let long_delay = Duration::from_millis(1000);

let mut peer1 = new_ephemeral(
decision_timeout,
Expand All @@ -42,20 +42,21 @@ async fn test_connection_breaks_after_timeout_without_decision() {
select! {
_ = peer1.next_swarm_event().fuse() => {},
_ = peer2.next_swarm_event().fuse() => {},
_ = sleep(large_delay).fuse() => {
_ = sleep(long_delay).fuse() => {
break;
}
}
}

// Connections should timeout without decisions.
assert!(!peer1.is_connected(peer2.local_peer_id()));
assert!(!peer2.is_connected(peer1.local_peer_id()));
}

#[tokio::test()]
async fn test_connection_decision() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other tests will all run for 2 seconds, are you sure we can't make it run MUCH faster? Like 100ms decision timeout and large delay of 200ms?

Copy link
Member Author

@shamil-gadelshin shamil-gadelshin Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 seconds are insignificant compared to the whole testing duration of the project. However, I can try setting it to 1s. Generally, I want to set delays several orders of magnitude higher than the expected overhead of all the scaffolding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is significant. Tests on my machine take ~5 minutes to run. 5 tests here will increase time to run tests by at least 2 seconds when running all in parallel, while not really using any meaningful amount of CPU. So having faster tests is certainly a desirable property. And in CI we only have 2 cores right now, so these 5 tests will take 6 seconds to run (depending on how they are scheduled). 3 seconds for 5 simple test cases is a lot.

let decision_timeout = Duration::from_millis(500);
let large_delay = Duration::from_millis(2000);
let decision_timeout = Duration::from_millis(300);
let long_delay = Duration::from_millis(1000);

let mut peer1 = new_ephemeral(
decision_timeout,
Expand Down Expand Up @@ -87,20 +88,21 @@ async fn test_connection_decision() {
select! {
_ = peer1.next_swarm_event().fuse() => {},
_ = peer2.next_swarm_event().fuse() => {},
_ = sleep(large_delay).fuse() => {
_ = sleep(long_delay).fuse() => {
break;
}
}
}

// Connections should be maintained after positive decisions.
assert!(peer1.is_connected(peer2.local_peer_id()));
assert!(peer2.is_connected(peer1.local_peer_id()));
}

#[tokio::test()]
async fn test_connection_decision_symmetry() {
let decision_timeout = Duration::from_millis(500);
let large_delay = Duration::from_millis(2000);
let decision_timeout = Duration::from_millis(300);
let long_delay = Duration::from_millis(1000);

let mut peer1 = new_ephemeral(
decision_timeout,
Expand Down Expand Up @@ -132,20 +134,21 @@ async fn test_connection_decision_symmetry() {
select! {
_ = peer1.next_swarm_event().fuse() => {},
_ = peer2.next_swarm_event().fuse() => {},
_ = sleep(large_delay).fuse() => {
_ = sleep(long_delay).fuse() => {
break;
}
}
}

// Both peers should approve the connection to make it permanent
assert!(!peer1.is_connected(peer2.local_peer_id()));
assert!(!peer2.is_connected(peer1.local_peer_id()));
}

#[tokio::test()]
async fn test_new_peer_request() {
let dialing_interval = Duration::from_millis(500);
let large_delay = Duration::from_millis(2000);
let dialing_interval = Duration::from_millis(300);
let long_delay = Duration::from_millis(1000);

let mut peer1 = new_ephemeral(
dialing_interval,
Expand All @@ -164,17 +167,19 @@ async fn test_new_peer_request() {
break;
}
},
_ = sleep(large_delay).fuse() => {
_ = sleep(long_delay).fuse() => {
panic!("No new peers requests.");
}
}
}

// We've received the new peers request when we don't have enough connected peers.
}

#[tokio::test()]
async fn test_target_connected_peer_limit_number() {
let decision_timeout = Duration::from_millis(500);
let large_delay = Duration::from_millis(2000);
let decision_timeout = Duration::from_millis(300);
let long_delay = Duration::from_millis(1000);
let target_connected_peers = 1;

let mut peer1 = new_ephemeral(
Expand Down Expand Up @@ -238,12 +243,13 @@ async fn test_target_connected_peer_limit_number() {
_ = peer1.next_swarm_event().fuse() => {},
_ = peer2.next_swarm_event().fuse() => {},
_ = peer3.next_swarm_event().fuse() => {},
_ = sleep(large_delay).fuse() => {
_ = sleep(long_delay).fuse() => {
break;
}
}
}

// We don't maintain with new peers when we have enough connected peers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helpful, but there are 3 blocks. The comment explains the high idea behind the whole test, it doesn't really comment what is happening here. And what is happening in these 3 blocks of assertions is checks where some peers are within limits and thus maintain connections and others are not and thus don't maintain connections.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment.

assert!(peer1.is_connected(peer2.local_peer_id()));
assert!(!peer1.is_connected(peer3.local_peer_id()));

Expand Down
Loading