use `net/mocknet` in integration tests #470

btc · 2014-12-24T02:34:23Z

This PR adds the net/mocknet to the tests/benchmarks that perform the Add/Cat operation.

@jbenet @whyrusleeping

TODOs:

remove panics
revise and cleanup
does mocknet require cleanup? mn.Close()?
when setting up mocknet, is it sufficient to link, but not connect peers?
private key generation is really expensive/slows down tests. got any ideas for faster fixture generation that meets mocknet requirements? Would @whyrusleeping's parameterized version solve this problem?
bootstrap DHT

jbenet · 2014-12-24T05:13:30Z

@maybebtc is this LFCR ?

btc · 2014-12-24T13:52:41Z

epictest/addcat_test.go

+			if _, err := mn.LinkPeers(i, j); err != nil {
+				return err
+			}
+			if err := mn.ConnectPeers(i, j); err != nil {


It's necessary to link peers, but not necessary to connect them, right?

@jbenet

@maybebtc

Link means that a connection is possible. (think physical link)

Connect means "open a connection". (think a net.Conn)

So yeah, you dont have to link nodes together, if someone will try to find the node and Dial as the result of some other actions in the test.

btc · 2014-12-24T13:56:52Z

@jbenet RFCR please see questions on original post.

jbenet · 2014-12-24T14:04:28Z

does mocknet require cleanup? mn.Close()?

We can add one for good practice. But no, it's just a struct.

when setting up mocknet, is it sufficient to link, but not connect peers?

Depends on what your code will do. If you code will open connections itself then no need to connect. if your code assumes that nodes have a live connection, then yes. (e.g. dht needs a few nodes bootstrapped (connected already) and from there it can open more connections by itself).

private key generation is really expensive/slows down tests. got any ideas for faster fixture generation that meets mocknet requirements? Would @whyrusleeping's parameterized version solve this problem?

Mocknet itself doesnt need the keys itself. They're there because things outside using mocknet may assume they have public keys because a connection is open. So it depends on your test. (we could add a function if you want, but you'll have to either setup the nodes with things that behave like keys, make sure you dont use anything that expects keys to be there).

jbenet · 2014-12-24T14:15:26Z

util/testutil/peer.go

+	ID() peer.ID
+	PrivateKey() ci.PrivKey
+	PublicKey() ci.PubKey
+}


please rename this to something other than Peer. suggest just using PeerNetParams directly. I just got done removing Peer from the entire codebase. It will breed confusion and break things. Also, anything called Peer shouldn't "contain" addresses. the ephemeral + indeterminate state of addresses is a source of so, so much pain. (peers can have many, can gain + lose them, might have different mappings depending on where two peers are in the network, might have complete information, might selectively expose addresses to some peers, etc etc etc.)

Why not just use PeerNetParams ?

This is testutil.Peer. The name is pretty clear.

Why not just use PeerNetParams ?

Don't want to tie down callsites to a concrete type that's subject to change.

Package names are not global. they are rebound locally. tu.Peer is no longer so clear.

it's Peer. please, i'm specifically requesting that we do not expose things named Peer anymore that represent an ipfs' peer's Identity, and purport to carry addresses.

renamed 0172e4f

jbenet · 2014-12-24T14:17:30Z

LGTM other than:

rename Peer to something else.
rebase on top of new master (just merged dht fixes #473)

so it'll be easier to create another implementation using the new mocknet

…ersion License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

…kpeernet under the hood License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

@whyrusleeping

https://travis-ci.org/jbenet/go-ipfs/jobs/45000756 cc @whyrusleeping @jbenet lol this is starting to happen pretty often

@whyrusleeping

@whyrusleeping @jbenet this is a WIP with the DHT. wip License: MIT Signed-off-by: Brian Tiger Chow <[email protected]> Conflicts: epictest/addcat_test.go exchange/bitswap/testnet/peernet.go exchange/bitswap/testutils.go routing/mock/centralized_server.go routing/mock/centralized_test.go routing/mock/interface.go fix(routing/mock) fill in function definition

@jbenet

pprof cannot be used reliably on OS X. This provides two make tasks to collect and analyze profiling data. To run profiling in a dockerized linux environment... ``` make // or `make collect` ``` To analyze results on host machine... ``` make analyze ``` @jbenet @whyrusleeping

@jbenet

It's now possible to produce the DHT issues without process orchestration. Test1KBInstantaneous fails @jbenet @whyrusleeping

@jbenet

cc @jbenet

btc · 2014-12-24T15:11:44Z

We can add one for good practice. But no, it's just a struct.

👍
Just want to make sure I'm not leaking goroutines and slowing down tests.

jbenet · 2014-12-24T15:22:18Z

thanks! lgtm

btc · 2014-12-24T15:24:33Z

still has two TODOs

faster key generation
bootstrap the DHT

whyrusleeping · 2014-12-24T18:35:47Z

@maybebtc my key generation using u.NewTimeSeededRand works a good deal faster. We should be using that for all keys generated in tests.

whyrusleeping · 2014-12-24T18:36:30Z

(I do this in my branch provides-rewrite and running a full test suite is tolerable on my chromebook now)

btc · 2014-12-25T06:41:46Z

@jbenet @whyrusleeping the test reproduces the docker network test failure. context deadline exceeded when adding and catting through a bootstrap node.

ready for merge

whyrusleeping · 2014-12-25T06:49:08Z

This looks good to me, the peer generation stuff gets some work in my branch, but as you said, we might want to merge this one first.

refactor(epictest) Core refactor: extract repo fix move core

btc · 2014-12-25T09:10:51Z

merging in

NB: this triggers go test failures

use `net/mocknet` in integration tests

…com/multiformats/go-multiaddr-0.2.1 build(deps): bump github.com/multiformats/go-multiaddr from 0.2.0 to 0.2.1

btc added the status/in-progress In progress label Dec 24, 2014

btc mentioned this pull request Dec 24, 2014

add mocknet to addcat test #461

Closed

btc force-pushed the feat/use-mocknet branch from 771ef44 to ac624da Compare December 24, 2014 13:32

btc reviewed Dec 24, 2014
View reviewed changes

jbenet reviewed Dec 24, 2014
View reviewed changes

btc force-pushed the feat/use-mocknet branch from ac624da to f9a8040 Compare December 24, 2014 14:28

Brian Tiger Chow added 20 commits December 24, 2014 09:31

misc docs and fmting

c933d43

feat(testutil) add testutil.Peer shim

b31f728

fix(bitswap) remove peerstore

81a6733

refactor: rename testutil func

a85aa9b

use testutil peer in sess

b551e45

add RandPeerOrFatal test helper

12b92f0

refactor(bitswap/testnet) slim down interface

e017d6e

so it'll be easier to create another implementation using the new mocknet

pass peer into testnet adapter method

6cdb4c2

ignore log output

d5e936a

refactor(bitswap/testnet) extract interface in prep for mockpeernet v…

d390e31

…ersion License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

refactor(bitswap/testnet) rename to virtual

85ce94c

License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

feat(bitswap/testnet) impl a version of bitswap testnet that uses moc…

6da57c6

…kpeernet under the hood License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>

refactor(routing/mock) move files

c253590

fix(merkledag/test) panic!

14990bb

https://travis-ci.org/jbenet/go-ipfs/jobs/45000756 cc @whyrusleeping @jbenet lol this is starting to happen pretty often

test(integration)

35f6ddb

It's now possible to produce the DHT issues without process orchestration. Test1KBInstantaneous fails @jbenet @whyrusleeping

cancel on exit

d619e4a

rm print

075841c

don't link when creating network client. rely on caller

237e594

rename to dht

2b2ff54

btc force-pushed the feat/use-mocknet branch from f9a8040 to 9117906 Compare December 24, 2014 14:53

style(testutil) rename testutil.Peer -> testutil.Identity

0172e4f

cc @jbenet

btc force-pushed the feat/use-mocknet branch from 9117906 to 0172e4f Compare December 24, 2014 14:59

add todo

b5fd310

Brian Tiger Chow added 2 commits December 25, 2014 01:17

rename variable

589ed37

use raw primitives

217611c

btc mentioned this pull request Dec 25, 2014

rewrite of provides to better select peers to send RPCs to #456

Merged

Brian Tiger Chow added 3 commits December 25, 2014 04:07

extract initialization

abd971d

refactor(epictest) Core refactor: extract repo fix move core

feat: test the three-legged cat

1891c89

rename

513c568

btc force-pushed the feat/use-mocknet branch from 1784d1e to 513c568 Compare December 25, 2014 09:07

btc pushed a commit that referenced this pull request Dec 25, 2014

Merge pull request #470 from jbenet/feat/use-mocknet

95ba056

use `net/mocknet` in integration tests

btc merged commit 95ba056 into master Dec 25, 2014

btc removed the status/in-progress In progress label Dec 25, 2014

btc deleted the feat/use-mocknet branch December 25, 2014 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `net/mocknet` in integration tests #470

use `net/mocknet` in integration tests #470

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

btc Dec 24, 2014

jbenet Dec 24, 2014

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

jbenet Dec 24, 2014

btc Dec 24, 2014

jbenet Dec 24, 2014

btc Dec 24, 2014

jbenet commented Dec 24, 2014

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

btc commented Dec 24, 2014

whyrusleeping commented Dec 24, 2014

whyrusleeping commented Dec 24, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

use net/mocknet in integration tests #470

use net/mocknet in integration tests #470

Conversation

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

btc Dec 24, 2014

Choose a reason for hiding this comment

jbenet Dec 24, 2014

Choose a reason for hiding this comment

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

jbenet Dec 24, 2014

Choose a reason for hiding this comment

btc Dec 24, 2014

Choose a reason for hiding this comment

jbenet Dec 24, 2014

Choose a reason for hiding this comment

btc Dec 24, 2014

Choose a reason for hiding this comment

jbenet commented Dec 24, 2014

btc commented Dec 24, 2014

jbenet commented Dec 24, 2014

btc commented Dec 24, 2014

whyrusleeping commented Dec 24, 2014

whyrusleeping commented Dec 24, 2014

btc commented Dec 25, 2014

whyrusleeping commented Dec 25, 2014

btc commented Dec 25, 2014

use `net/mocknet` in integration tests #470

use `net/mocknet` in integration tests #470