Tests for HandledNodesTasks #584

dvdplm · 2018-10-23T12:22:09Z

Based on #546

…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)

…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)

…andled_node * dp/chore/test-core-listeners: Prefer freestanding function Address grumbles Remove non-project specific stuff Address grumbles

Add HandlerState and use it in TestBuilder Shorter test names

Test NotReady case

Track events as they reach the Handler Describe complex test logic

…nt contents More tests for poll()

…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)

tomaka · 2018-11-02T15:22:39Z

Please remove the printlns, the diff is really verbose.

dvdplm · 2018-11-02T16:17:58Z

Done.

tomaka · 2018-11-05T10:28:58Z

Just in general I think it's bad to have style changes in the same PR as something else (or even in general two different changes in the same PR). It's tolerable when there are two or three style changes, but not more. I'm not going to ask you to revert the style changes, but this really pollute the diff.

core/src/nodes/handled_node.rs

core/src/nodes/handled_node_tasks.rs

tomaka · 2018-11-05T10:35:38Z

core/src/nodes/handled_node_tasks.rs

+
+            // Even though we set up the muxer outbound state to be `Closed` we
+            // still need to open a substream or the outbound state will never
+            // be checked.


This should be fixed in the non-test code.

I'll look into it, but I suspect that it's more of an artifact of the DummyMuxer than a fault of the actual code.

I am wrong and you're right (and the comment is correct); the NodeStream does not check if the outbound state is open or closed (it does check the inbound state).

I'll open an issue and fix in separate PR, ok?

core/src/nodes/handled_node_tasks.rs

dvdplm · 2018-11-05T19:04:45Z

ust in general I think it's bad to have style changes in the same PR as something else (or even in general two different changes in the same PR).

I agree with this, mixing things makes reviewing much harder than it needs to be.
I am not sure which style changes you refer to here, but I did run rustfmt on two of the test helper files (dummy_transport and dummy_muxer I think) which I think is ok, as the changes there are all relevant to the tests added here. I'll try to pay close attention to it going forward though.

Test Task.send_event() and id() using a HandledNodesTasks

…e-handled_node_tasks * upstream/master: Inject event by value in ProtocolsHandler (libp2p#605) Add a PeriodicPingHandler and a PingListenHandler (libp2p#574) Fix stack overflow when printing a SubstreamRef (libp2p#599) Add a peer id generator (libp2p#583) eg. -> e.g.; ie. -> i.e. via repren (libp2p#592)

…e-handled_node_tasks * upstream/master: Use vecdeque for fair polling (libp2p#610) Add back the Swarm (libp2p#613) Remove dependency on tokio_current_thread (libp2p#606) Use chashmap from crates (libp2p#615) Fix grammar in core/nodes etc. (libp2p#608)

dvdplm · 2018-11-13T08:56:06Z

@tomaka I believe there are two outstanding issues here:

Debug impl for HandledNodesTasks: do you like the suggested change there?
test organization in submodules: do you want me to re-work that?

Other than the above, is there anything else I can do to move this forward?

tomaka · 2018-11-13T16:56:54Z

Yes, please remove the hierarchy of sub-modules in the tests.

I disagree with the surprise factor for changing the Debug implementation. This struct is a collection of tasks, and every single collection in the Rust standard library uses this debug output. To me the reason why most Debug implementations simply print the struct layout is because programmers are lazy, not because it's the right thing to do.
Let's make @twittner decide?

dvdplm · 2018-11-14T09:11:27Z

Let's make @twittner decide?

I'll just make the change you prefer and if someone disagrees vehemently we can discuss it again. :)

…e-handled_node_tasks * upstream/master: Tweaks, spelling and grammar (libp2p#629) Add a badge with a link to deps.rs (libp2p#630) Rewrite floodsub to use the ProtocolsHandler (libp2p#603) Add an IdentifyListen behaviour (libp2p#626) Add a custom derive for NetworkBehaviour (libp2p#619) Set the maximum size of Mplex messages to 1Mb (libp2p#622) Use expect rather than unwrap (libp2p#625) Make libp2p-websocket optional (libp2p#624) Add From<IpAddr> for Multiaddr (libp2p#623) Add implementations of NetworkBehaviour for ping (libp2p#618) Add a PeriodicIdentifyBehaviour (libp2p#617) Use upstream rust-secp256k1 (libp2p#616) Use yamux and aio-limited from crates.io (libp2p#621)

dvdplm · 2018-11-14T09:27:04Z

@tomaka ptal

…lection_stream * upstream/master: Fix the concerns of libp2p#603 (libp2p#635) Tests for HandledNodesTasks (libp2p#584)

…ample * upstream/master: Fix the concerns of libp2p#603 (libp2p#635) Tests for HandledNodesTasks (libp2p#584) Tweaks, spelling and grammar (libp2p#629) Add a badge with a link to deps.rs (libp2p#630)

…_swarm * upstream/master: (33 commits) Behavior -> Behaviour (libp2p#650) Remove Send/Sync/'static reqs from ping (libp2p#652) Tests for CollectionStream (libp2p#588) Document the muxing module (libp2p#647) Document the core::upgrade module (libp2p#644) Send Close when shutting down gracefully, as per spec (libp2p#631) Remove the Send + 'static requirements for floodsub (libp2p#637) Move protocols_handler to the root (libp2p#643) Use a random sequence number in floodsub (libp2p#634) Split `ConnectionUpgrade`. (libp2p#642) Add an impl Debug for HandledNode (libp2p#628) minor typo in docs (libp2p#640) Update connected peers' topics on NodeEvent (libp2p#638) Fix the concerns of libp2p#603 (libp2p#635) Tests for HandledNodesTasks (libp2p#584) Tweaks, spelling and grammar (libp2p#629) Add a badge with a link to deps.rs (libp2p#630) Rewrite floodsub to use the ProtocolsHandler (libp2p#603) Add an IdentifyListen behaviour (libp2p#626) Add a custom derive for NetworkBehaviour (libp2p#619) ...

dvdplm added 30 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)

Tests for HandledNode

182034f

Address grumbles

9a10593

Remove non-project specific stuff

9d8ebc1

Address grumbles

3cb89fd

Prefer freestanding function

b83ca9f

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

1297208

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

a0c15c6

Merge branch 'dp/chore/test-core-listeners' into dp/chore/test-core-h…

28bc967

…andled_node * dp/chore/test-core-listeners: Prefer freestanding function Address grumbles Remove non-project specific stuff Address grumbles

Untangle the code for old shutdown test from the new tests

c7cdd8d

Add HandlerState and use it in TestBuilder Shorter test names

WIP – tests pass

1947a79

Use a newtype to lighten up the function signatures a bit

f0a714b

Test NotReady case

Cleanup Event enum

f6807d3

Track events as they reach the Handler Describe complex test logic

Assert on the event trace

71a720a

More tests for poll()

883c8f2

Switch to using usize as the OutboundOpenInfo so we can assert on eve…

df74ff8

…nt contents More tests for poll()

dvdplm added 2 commits November 2, 2018 17:13

Less noise

d3bd80f

cleanup

a4ebe7f

tomaka suggested changes Nov 5, 2018

View reviewed changes

dvdplm commented Nov 5, 2018

View reviewed changes

core/src/nodes/handled_node_tasks.rs Outdated Show resolved Hide resolved

dvdplm added 4 commits November 6, 2018 11:00

Address grumbles

6749d8b

Better debug impl for HandledNodesTasks

261f4cc

Remove tests for Task we don't need

d9dd39c

Test Task.send_event() and id() using a HandledNodesTasks

dvdplm mentioned this pull request Nov 6, 2018

Fix for corner case where the outbound state of a NodeStream is never checked until a substream is opened #611

Closed

dvdplm mentioned this pull request Nov 9, 2018

Tests for CollectionStream #588

Merged

dvdplm added 5 commits November 14, 2018 10:11

Rename test builders

6ac3f8e

Don't organise tests in submodules

8d5442f

whitespace

2a2cad3

Revert changes to Debug impl for HandledNodesTasks

58e9ae6

tomaka approved these changes Nov 14, 2018

View reviewed changes

tomaka merged commit 513b97b into libp2p:master Nov 14, 2018

dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Nov 14, 2018

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

fe4cd33

…lection_stream * upstream/master: Fix the concerns of libp2p#603 (libp2p#635) Tests for HandledNodesTasks (libp2p#584)

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 HandledNodesTasks #584

Tests for HandledNodesTasks #584

dvdplm commented Oct 23, 2018

tomaka commented Nov 2, 2018

dvdplm commented Nov 2, 2018

tomaka commented Nov 5, 2018

tomaka Nov 5, 2018

dvdplm Nov 5, 2018

dvdplm Nov 6, 2018

dvdplm Nov 6, 2018

dvdplm commented Nov 5, 2018

dvdplm commented Nov 13, 2018

tomaka commented Nov 13, 2018 •

edited

Loading

dvdplm commented Nov 14, 2018

dvdplm commented Nov 14, 2018

Tests for HandledNodesTasks #584

Tests for HandledNodesTasks #584

Conversation

dvdplm commented Oct 23, 2018

tomaka commented Nov 2, 2018

dvdplm commented Nov 2, 2018

tomaka commented Nov 5, 2018

tomaka Nov 5, 2018

Choose a reason for hiding this comment

dvdplm Nov 5, 2018

Choose a reason for hiding this comment

dvdplm Nov 6, 2018

Choose a reason for hiding this comment

dvdplm Nov 6, 2018

Choose a reason for hiding this comment

dvdplm commented Nov 5, 2018

dvdplm commented Nov 13, 2018

tomaka commented Nov 13, 2018 • edited Loading

dvdplm commented Nov 14, 2018

dvdplm commented Nov 14, 2018

tomaka commented Nov 13, 2018 •

edited

Loading