Tests for nodes/listeners.rs #541

dvdplm · 2018-10-05T15:08:18Z

The relevant commits here are:

ec2a2f1: unit tests
6341575: extra tests to illustrate/facilitate upcoming refactor

…e-listeners * upstream/master: 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)

…isteners

…e-listeners * upstream/master: Add unit tests for core::nodes::NodeStream (libp2p#535)

twittner · 2018-10-09T11:41:13Z

core/src/tests/dummy_transport.rs

+						(Box::new(stream), addr2)
+					},
+					Async::Ready(None) => {
+						let stream = stream::poll_fn(|| future::ok(None).poll() )


let stream = stream::empty()?

That is indeed a lot better. :)

twittner · 2018-10-09T11:44:32Z

core/src/tests/dummy_transport.rs

+						(Box::new(stream), addr2)
+					},
+					Async::Ready(Some(_)) => {
+						let stream = stream::poll_fn(|| future::ok(Some(1usize)).poll() )


let stream = stream::repeat(1)?

I think it would be more useful even to enumerate the events instead of constantly yielding 1, so how about:

Async::Ready(Some(n)) => { let stream = stream::iter_ok(n..) ...

I like it, much clearer.

twittner · 2018-10-09T11:48:53Z

core/src/tests/dummy_transport.rs

+				let tupelize = move |stream| future::ok( (stream, future::ok(addr.clone())) );
+				Ok(match async {
+					Async::NotReady => {
+						let stream = stream::poll_fn(|| future::empty().poll() )


let stream = stream::poll_fn(|| Ok(Async::NotReady))?

+1

As you can tell I struggled quite a bit to get the type-soup right here. Great suggestions, much appreciated.

twittner · 2018-10-09T11:49:28Z

core/src/tests/dummy_transport.rs

+			}
+			ListenerState::Error => Err( (self, addr2) )
+		}
+		}


This } looks misaligned.

twittner · 2018-10-09T12:11:15Z

core/src/tests/dummy_transport.rs

+
+#[derive(Debug, PartialEq, Clone, Copy)]
+pub(crate) enum ListenerState {
+	Ok(Async<Option<usize>>),


A comment about the usize here could be useful.

The idea here was just to put "something" in there, preferably non-void. I see why it could be surprising to the reader; what's a better choice? I did add a comment though?

Initially I assumed it would be a dummy, in which case I would have suggested () instead. However one can write tests which check the item index of a listener (hence my other suggestion (stream::iter_ok(n..)). Consequently I would write a comment stating that the usize value indexes items produced by the listener.

twittner · 2018-10-09T12:23:09Z

core/src/nodes/listeners.rs

+    // set things up for the tests.
+    impl ListenersStream<DummyTransport> {
+        fn set_listener_state(&mut self, idx: usize, state: ListenerState) {
+            let mut l = self.listeners.remove(idx);


Why removing and re-inserting? Why not just let l = &mut self.listeners[idx];?

Mostly to satisfy the compiler (got a error[E0507]: cannot move out of indexed content otherwise) but when I wrote it I was trying to understand the re-ordering behaviour of the code so I thought I'd make it behave "realistically". I think it is necessary though?

let l = &mut self.listeners[idx]; should work.

…e-listeners * upstream/master: Fix secio compied with --no-default-features (libp2p#545) multiaddr: explain the use of `&str` for `Unix`. (libp2p#543) Model HandshakeContext with explicit state transitions. (libp2p#532) multiaddr: add support for onion protocol. (libp2p#542)

twittner · 2018-10-09T15:13:31Z

core/src/nodes/listeners.rs

+                                Box::new(stream)
+                            }
+                        }
+                    }


Should the ListenerState::Ok branch of this match not be doing the same as in DummyTransport::listen_on?

Definitely should, fixing.

tomaka · 2018-10-05T15:43:45Z

.gitignore

@@ -1,2 +1,4 @@
 target
 Cargo.lock
+.idea/**


The content of gitignore should be specific to the project => https://stackoverflow.com/questions/7335420/global-git-ignore

Yes, this was a mistake. Reverting.

tomaka · 2018-10-09T09:26:33Z

core/src/nodes/listeners.rs

+    // Test helper that lets us poke in innards of individual `Listener`s and
+    // set things up for the tests.
+    impl ListenersStream<DummyTransport> {
+        fn set_listener_state(&mut self, idx: usize, state: ListenerState) {


I'd prefer a freestanding method that accepts a &mut ListenersStream<DummyTransport>.
We don't want to hide method implementations behind #[cfg(test)].

tomaka · 2018-10-09T09:29:57Z

core/src/nodes/listeners.rs

+        ls.listen_on(addr1).expect("listen_on failed");
+        ls.listen_on(addr2).expect("listen_on failed");
+
+        let listener_addrs = ls.listeners().map(|ma| ma.to_string() ).collect::<Vec<String>>();


Theoretically shouldn't have to convert to string, but why not.

The trouble was that the iterator doesn't impl Debug (and PartialEq) so I did this to move ahead. Should I try to improve on this or not worth it?

Not worth it since it's just a test.

tomaka · 2018-10-09T09:30:59Z

core/src/tests/dummy_transport.rs

+
+#[derive(Debug, PartialEq, Clone, Copy)]
+pub(crate) enum ListenerState {
+	Ok(Async<Option<usize>>),


Indentation should be 4 spaces.

Oh, so it's spaces on libp2p, interesting! :)

Is it ok if I add an .editorconfig to the project?

tomaka · 2018-10-09T09:32:11Z

core/src/tests/dummy_transport.rs

+			}
+			ListenerState::Error => Err( (self, addr2) )
+		}
+		}


Indentation

…e-handled_node * upstream/master: Add `StreamMuxer::flush`. (libp2p#534) Tests for nodes/listeners.rs (libp2p#541) Deny relative addresses for UNIX domain sockets (libp2p#548) Optimise `Multiaddr::append`. (libp2p#549)

…pl-for-node-event * upstream/master: Shut down yamux and fix mplex shutdown. (libp2p#559) Add `StreamMuxer::flush`. (libp2p#534) Tests for nodes/listeners.rs (libp2p#541) Deny relative addresses for UNIX domain sockets (libp2p#548) Optimise `Multiaddr::append`. (libp2p#549)

dvdplm added 13 commits October 3, 2018 11:16

Add unit tests for core::nodes::NodeStream

71fa660

Move DummyMuxer to core/tests

3b95906

Address grumbles

12bbc06

Impl Debug for SubstreamRef<P>

364e59a

Add test for poll()

924259d

Don't need to open a substream

5e60d85

pretty printer test

caf9a86

More tests for NodeStream poll()

4d98f65

ListenerStream unit tests: transport() and listeners()

a894aac

Tests for nodes/listeners.rs

ec2a2f1

Add a few tests to help illustrate the "drowning" behaviour of busy l…

6341575

…isteners

Merge remote-tracking branch 'upstream/master' into dp/chore/test-cor…

961ca7f

…e-listeners * upstream/master: Add unit tests for core::nodes::NodeStream (libp2p#535)

twittner reviewed Oct 9, 2018

View reviewed changes

dvdplm added 2 commits October 9, 2018 16:26

Address grumbles

9a10593

twittner reviewed Oct 9, 2018

View reviewed changes

tomaka suggested changes Oct 9, 2018

View reviewed changes

dvdplm added 3 commits October 10, 2018 08:59

Remove non-project specific stuff

9d8ebc1

Address grumbles

3cb89fd

Prefer freestanding function

b83ca9f

twittner approved these changes Oct 10, 2018

View reviewed changes

Merge branch 'master' into dp/chore/test-core-listeners

1297208

tomaka approved these changes Oct 10, 2018

View reviewed changes

tomaka added the pr-queued-to-merge label Oct 10, 2018

Merge branch 'master' into dp/chore/test-core-listeners

a17f6ca

tomaka merged commit 0c7f313 into libp2p:master Oct 10, 2018

tomaka mentioned this pull request Dec 7, 2018

Write tests for the code in core/nodes #509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for nodes/listeners.rs #541

Tests for nodes/listeners.rs #541

dvdplm commented Oct 5, 2018 •

edited

Loading

twittner Oct 9, 2018

dvdplm Oct 9, 2018

twittner Oct 9, 2018

dvdplm Oct 9, 2018

twittner Oct 9, 2018

dvdplm Oct 9, 2018

twittner Oct 9, 2018

twittner Oct 9, 2018

dvdplm Oct 9, 2018

twittner Oct 9, 2018

twittner Oct 9, 2018

dvdplm Oct 9, 2018

twittner Oct 9, 2018

twittner Oct 9, 2018

dvdplm Oct 10, 2018

tomaka Oct 5, 2018

dvdplm Oct 10, 2018

tomaka Oct 9, 2018

tomaka Oct 9, 2018

dvdplm Oct 10, 2018

tomaka Oct 10, 2018

tomaka Oct 9, 2018

dvdplm Oct 10, 2018

tomaka Oct 9, 2018

Tests for nodes/listeners.rs #541

Tests for nodes/listeners.rs #541

Conversation

dvdplm commented Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm commented Oct 5, 2018 •

edited

Loading