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.

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

Add unit tests for core::nodes::NodeStream #535

Merged
merged 10 commits into from
Oct 5, 2018

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Oct 3, 2018

No description provided.


#[test]
#[ignore]
fn opening_substream_notifies_task() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to test this code from open_substream():

        if let Some(task) = self.to_notify.take() {
            task.notify();
        }

Pointers appreciated!

Copy link
Member

Choose a reason for hiding this comment

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

The to_notify has been removed in the master branch.

@@ -86,7 +86,16 @@ enum Addr<TAddrFut> {
/// A successfully opened substream.
pub type Substream<TMuxer> = muxing::SubstreamRef<Arc<TMuxer>>;

impl<TMuxer> fmt::Debug for Substream<TMuxer>
Copy link
Member

Choose a reason for hiding this comment

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

This should be implemented on muxing::SubstreamRef, not on this type.

extern crate futures;

use std::io::Error as IoError;
use ::muxing::StreamMuxer;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't use a :: prefix.

_ => unimplemented!()
}
}
#[allow(unused_variables)]
Copy link
Member

Choose a reason for hiding this comment

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

Don't use this allow. Replace the unused parameter names with underscores instead.

#[allow(unused_variables)]
fn destroy_outbound(&self, substream: Self::OutboundSubstream) {}
#[allow(unused_variables)]
fn read_substream(&self, substream: &mut Self::Substream, buf: &mut [u8]) -> Result<usize, IoError> { Ok(1) }
Copy link
Member

Choose a reason for hiding this comment

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

Use unreachable!() instead of returning a wrong value. Same for all the methods that shouldn't be called.


#[derive(Debug)]
pub struct DummyMuxer{
pub in_connection: DummyConnection,
Copy link
Member

Choose a reason for hiding this comment

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

You should either have setters or public fields, but not both.


#[derive(Debug, PartialEq)]
pub enum DummyConnectionState {
Substream,
Copy link
Member

Choose a reason for hiding this comment

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

What do the Substream and Open variants mean? Once a substream is open, the substream is yielded as a DummySubstream and the state should reset to Pending.

@dvdplm
Copy link
Contributor Author

dvdplm commented Oct 4, 2018

@tomaka ptal

Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Let's wait to see if there's a discussion in #538 about testing the debug output, and then either merge or remove the prints_pretty_debug_info test based on that.

}

#[test]
fn prints_pretty_debug_info() {
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 the only contention point. I'm not totally sure we want to test the output of the Debug implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll just remove it from here and if/when #538 lands it'll be there (or not).

@tomaka tomaka merged commit d81f4e2 into libp2p:master Oct 5, 2018
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 5, 2018
…e-listeners

* upstream/master:
  Add unit tests for core::nodes::NodeStream (libp2p#535)
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Oct 5, 2018
…pl-for-node-event

* upstream/master:
  Add unit tests for core::nodes::NodeStream (libp2p#535)
  Implement GET_PROVIDERS/ADD_PROVIDER Kademlia messages (libp2p#530)
  Make secio almost compile for asmjs/wasm (libp2p#519)
  Make it possible to accept/deny nodes in CollectionsStream (libp2p#512)
  Fix polling in handled_node.rs (libp2p#531)
  Remove notifying tasks (libp2p#528)
  Fix handler not properly shut down, and write test for this (libp2p#514)
  Use unsigned-varints, add BLAKE2 support in multihash (libp2p#525)
  Upgrade smallvec version to fix license issue (libp2p#526)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants