-
Notifications
You must be signed in to change notification settings - Fork 226
NDRS-119: add deploy gossiper tests #87
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.
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
Merged
Fraser999
merged 10 commits into
casper-network:master
from
Fraser999:NDRS-119-deploy-gossiper
Jul 17, 2020
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
879d1a2
NDRS-119: add deploy gossiper tests
Fraser999 ddb316b
NDRS-119: remove stray commented line
Fraser999 06258ea
Merge branch 'master' into NDRS-119-deploy-gossiper
Fraser999 259c921
NDRS-119: refactor Runner::inject_event_on to make it less prone to a…
Fraser999 1241296
NDRS-119: added 'crank_until' to test network using newtype reactor w…
Fraser999 9657956
Merge branch 'master' into NDRS-119-deploy-gossiper
Fraser999 0d3ad3a
NDRS-119: change unwrap for expect
Fraser999 2154290
Merge remote-tracking branch 'upstream/master' into NDRS-119-deploy-g…
Fraser999 2e796dd
NDRS-119: ensure test timeouts panic to avoid potentially invalidatin…
Fraser999 1ba0dc7
NDRS-119: refactor random test variables to chosen values
Fraser999 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| use std::{ | ||
| collections::HashSet, | ||
| fmt::{self, Display, Formatter}, | ||
| }; | ||
|
|
||
| use super::Message; | ||
| use crate::{ | ||
| components::{small_network::NodeId, storage}, | ||
| types::{Deploy, DeployHash}, | ||
| utils::DisplayIter, | ||
| }; | ||
|
|
||
| /// `DeployGossiper` events. | ||
| #[derive(Debug)] | ||
| pub enum Event { | ||
| /// A new deploy has been received to be gossiped. | ||
| DeployReceived { deploy: Box<Deploy> }, | ||
| /// The network component gossiped to the included peers. | ||
| GossipedTo { | ||
| deploy_hash: DeployHash, | ||
| peers: HashSet<NodeId>, | ||
| }, | ||
| /// The timeout for waiting for a gossip response has elapsed and we should check the response | ||
| /// arrived. | ||
| CheckGossipTimeout { | ||
| deploy_hash: DeployHash, | ||
| peer: NodeId, | ||
| }, | ||
| /// The timeout for waiting for the full deploy body has elapsed and we should check the | ||
| /// response arrived. | ||
| CheckGetFromPeerTimeout { | ||
| deploy_hash: DeployHash, | ||
| peer: NodeId, | ||
| }, | ||
| /// An incoming gossip network message. | ||
| MessageReceived { sender: NodeId, message: Message }, | ||
| /// The result of the `DeployGossiper` putting a deploy to the storage component. If the | ||
| /// result is `Ok`, the deploy hash should be gossiped onwards. | ||
| PutToStoreResult { | ||
| deploy_hash: DeployHash, | ||
| maybe_sender: Option<NodeId>, | ||
| result: storage::Result<bool>, | ||
| }, | ||
| /// The result of the `DeployGossiper` getting a deploy from the storage component. If the | ||
| /// result is `Ok`, the deploy should be sent to the requesting peer. | ||
| GetFromStoreResult { | ||
| deploy_hash: DeployHash, | ||
| requester: NodeId, | ||
| result: Box<storage::Result<Deploy>>, | ||
| }, | ||
| } | ||
|
|
||
| impl Display for Event { | ||
| fn fmt(&self, formatter: &mut Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Event::DeployReceived { deploy } => { | ||
| write!(formatter, "new deploy received: {}", deploy.id()) | ||
| } | ||
| Event::GossipedTo { deploy_hash, peers } => write!( | ||
| formatter, | ||
| "gossiped {} to {}", | ||
| deploy_hash, | ||
| DisplayIter::new(peers) | ||
| ), | ||
| Event::CheckGossipTimeout { deploy_hash, peer } => write!( | ||
| formatter, | ||
| "check gossip timeout for {} with {}", | ||
| deploy_hash, peer | ||
| ), | ||
| Event::CheckGetFromPeerTimeout { deploy_hash, peer } => write!( | ||
| formatter, | ||
| "check get from peer timeout for {} with {}", | ||
| deploy_hash, peer | ||
| ), | ||
| Event::MessageReceived { sender, message } => { | ||
| write!(formatter, "{} received from {}", message, sender) | ||
| } | ||
| Event::PutToStoreResult { | ||
| deploy_hash, | ||
| result, | ||
| .. | ||
| } => { | ||
| if result.is_ok() { | ||
| write!(formatter, "put {} to store", deploy_hash) | ||
| } else { | ||
| write!(formatter, "failed to put {} to store", deploy_hash) | ||
| } | ||
| } | ||
| Event::GetFromStoreResult { | ||
| deploy_hash, | ||
| result, | ||
| .. | ||
| } => { | ||
| if result.is_ok() { | ||
| write!(formatter, "got {} from store", deploy_hash) | ||
| } else { | ||
| write!(formatter, "failed to get {} from store", deploy_hash) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| use std::fmt::{self, Display, Formatter}; | ||
|
|
||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use crate::types::{Deploy, DeployHash}; | ||
|
|
||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| pub enum Message { | ||
| /// Gossiped out to random peers to notify them of a `Deploy` we hold. | ||
| Gossip(DeployHash), | ||
| /// Response to a `Gossip` message. If `is_already_held` is false, the recipient should treat | ||
| /// this as a `GetRequest` and send a `GetResponse` containing the `Deploy`. | ||
| GossipResponse { | ||
| deploy_hash: DeployHash, | ||
| is_already_held: bool, | ||
| }, | ||
| /// Sent if a `Deploy` fails to arrive, either after sending a `GossipResponse` with | ||
| /// `is_already_held` set to false, or after a previous `GetRequest`. | ||
| GetRequest(DeployHash), | ||
| /// Sent in response to a `GetRequest`, or to a peer which responded to gossip indicating it | ||
| /// didn't already hold the full `Deploy`. | ||
| GetResponse(Box<Deploy>), | ||
| } | ||
|
|
||
| impl Display for Message { | ||
| fn fmt(&self, formatter: &mut Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Message::Gossip(deploy_hash) => write!(formatter, "gossip({})", deploy_hash), | ||
| Message::GossipResponse { | ||
| deploy_hash, | ||
| is_already_held, | ||
| } => write!( | ||
| formatter, | ||
| "gossip-response({}, {})", | ||
| deploy_hash, is_already_held | ||
| ), | ||
| Message::GetRequest(deploy_hash) => write!(formatter, "get-request({})", deploy_hash), | ||
| Message::GetResponse(deploy) => write!(formatter, "get-response({})", deploy.id()), | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why this is desired, but I believe I removed it because it made it not show up in the docs, IIRC. Also, I do not think
doctestsare executed in this case.Correct me if I am from, but this should be conditional on both
testanddoctest? (#[cfg(any(test, doctest))])There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the consensus so far appears to be that we shouldn't add modules/types to public APIs unless required. After the proposed style guide is finalised, if we do prefer to extend APIs just to support docs/doctests we can change this, but for now I'd rather keep the test-only code test feature-gated.
If we take that view, I don't think there's any point to changing this to
#[cfg(any(test, doctest))]since doctests won't run this code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doctestwill cause it to run withcargo testAFAIK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tried and it seems like it doesn't.