-
Notifications
You must be signed in to change notification settings - Fork 112
bzzeth: protocol runloop now quits on peer disconnect #1745
Conversation
bzzeth/bzzeth_test.go
Outdated
// Wait for sometime and see if peer is connected and idling | ||
time.Sleep(1000 * time.Millisecond) | ||
if _, ok := b.peers.peers[node.ID()]; !ok { | ||
t.Fatalf("peer is not connected") |
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.
This test seems flaky now:
ERROR[09-11|15:53:03.329|github.com/ethersphere/swarm/p2p/protocols/protocol.go:238] peer.handleIncoming err="Message handler error: (msg code 1): received message from Swarm node"
--- FAIL: TestBzzBzzHandshake (1.09s)
bzzeth_test.go:162: peer still connected
FAIL
coverage: 46.0% of statements
FAIL github.com/ethersphere/swarm/bzzeth 2.364s
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 added some delay after sending the dummy message... It should work fine now.
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 know that we are trying to avoid adding sleeps to tests. I it possible to fix flakiness with other methods?
BTW, I do not see the new delay, did you push?
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.
Pushed it now
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.
Sleep seems to be the option for now...
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 would like to hear other opinions on using sleeps.
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.
There is a logical error in TestBzzBzzHandshakeWithMessage condition check causing the test to fail.
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.
Thanks Zahoor! LGTM.
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.
approved but #1756
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if _, ok := b.peers.peers[node.ID()]; ok { | ||
t.Fatalf("peer still connected") | ||
// after successful handshake, expect peer added to peer pool |
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.
wrong comment
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.
there is a disconnected call on the tester.
t.Fatalf("peer is not connected") | ||
// after successful handshake, expect peer added to peer pool | ||
var p *Peer | ||
for i := 0; i < 10; i++ { |
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.
dry up code
|
||
func isPeerDisconnected(id enode.ID, b *BzzEth) (p *Peer) { |
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.
imprecise, this is testing if peer is not in testpool
fixes #1730