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.

Sign up for GitHub

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

p2p, swarm: remove unused testing.T in protocol tester #18500

Merged
merged 3 commits into from
Jan 24, 2019
Merged

p2p, swarm: remove unused testing.T in protocol tester #18500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ef8184
p2p, swarm: remove unused testing.T in protocol tester
acud Jan 16, 2019
97936bc
Merge branch 'master' into p2p-testutil-unused-t
acud Jan 18, 2019
3678dfc
swarm/network: gofmt
nonsense Jan 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions p2p/protocols/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ func newProtocol(pp *p2ptest.TestPeerPool) func(*p2p.Peer, p2p.MsgReadWriter) er
}
}

func protocolTester(t *testing.T, pp *p2ptest.TestPeerPool) *p2ptest.ProtocolTester {
func protocolTester(pp *p2ptest.TestPeerPool) *p2ptest.ProtocolTester {
conf := adapters.RandomNodeConfig()
return p2ptest.NewProtocolTester(t, conf.ID, 2, newProtocol(pp))
return p2ptest.NewProtocolTester(conf.ID, 2, newProtocol(pp))
}

func protoHandshakeExchange(id enode.ID, proto *protoHandshake) []p2ptest.Exchange {
Expand Down Expand Up @@ -173,7 +173,7 @@ func protoHandshakeExchange(id enode.ID, proto *protoHandshake) []p2ptest.Exchan

func runProtoHandshake(t *testing.T, proto *protoHandshake, errs ...error) {
pp := p2ptest.NewTestPeerPool()
s := protocolTester(t, pp)
s := protocolTester(pp)
// TODO: make this more than one handshake
node := s.Nodes[0]
if err := s.TestExchanges(protoHandshakeExchange(node.ID(), proto)...); err != nil {
Expand Down Expand Up @@ -250,7 +250,7 @@ func TestProtocolHook(t *testing.T) {
}

conf := adapters.RandomNodeConfig()
tester := p2ptest.NewProtocolTester(t, conf.ID, 2, runFunc)
tester := p2ptest.NewProtocolTester(conf.ID, 2, runFunc)
err := tester.TestExchanges(p2ptest.Exchange{
Expects: []p2ptest.Expect{
{
Expand Down Expand Up @@ -389,7 +389,7 @@ func moduleHandshakeExchange(id enode.ID, resp uint) []p2ptest.Exchange {

func runModuleHandshake(t *testing.T, resp uint, errs ...error) {
pp := p2ptest.NewTestPeerPool()
s := protocolTester(t, pp)
s := protocolTester(pp)
node := s.Nodes[0]
if err := s.TestExchanges(protoHandshakeExchange(node.ID(), &protoHandshake{42, "420"})...); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -469,7 +469,7 @@ func testMultiPeerSetup(a, b enode.ID) []p2ptest.Exchange {

func runMultiplePeers(t *testing.T, peer int, errs ...error) {
pp := p2ptest.NewTestPeerPool()
s := protocolTester(t, pp)
s := protocolTester(pp)

if err := s.TestExchanges(testMultiPeerSetup(s.Nodes[0].ID(), s.Nodes[1].ID())...); err != nil {
t.Fatal(err)
Expand Down
3 changes: 1 addition & 2 deletions p2p/testing/protocoltester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"io/ioutil"
"strings"
"sync"
"testing"

"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/node"
Expand All @@ -52,7 +51,7 @@ type ProtocolTester struct {
// NewProtocolTester constructs a new ProtocolTester
// it takes as argument the pivot node id, the number of dummy peers and the
// protocol run function called on a peer connection by the p2p server
func NewProtocolTester(t *testing.T, id enode.ID, n int, run func(*p2p.Peer, p2p.MsgReadWriter) error) *ProtocolTester {
func NewProtocolTester(id enode.ID, n int, run func(*p2p.Peer, p2p.MsgReadWriter) error) *ProtocolTester {
services := adapters.Services{
"test": func(ctx *adapters.ServiceContext) (node.Service, error) {
return &testNode{run}, nil
Expand Down
15 changes: 8 additions & 7 deletions swarm/network/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func newBzzBaseTester(t *testing.T, n int, addr *BzzAddr, spec *protocols.Spec,
return srv(&BzzPeer{Peer: protocols.NewPeer(p, rw, spec), BzzAddr: NewAddr(p.Node())})
}

s := p2ptest.NewProtocolTester(t, addr.ID(), n, protocol)
s := p2ptest.NewProtocolTester(addr.ID(), n, protocol)

for _, node := range s.Nodes {
cs[node.ID().String()] = make(chan bool)
Expand Down Expand Up @@ -116,9 +116,9 @@ func newBzz(addr *BzzAddr, lightNode bool) *Bzz {
return bzz
}

func newBzzHandshakeTester(t *testing.T, n int, addr *BzzAddr, lightNode bool) *bzzTester {
func newBzzHandshakeTester(n int, addr *BzzAddr, lightNode bool) *bzzTester {
bzz := newBzz(addr, lightNode)
pt := p2ptest.NewProtocolTester(t, addr.ID(), n, bzz.runBzz)
pt := p2ptest.NewProtocolTester(addr.ID(), n, bzz.runBzz)

return &bzzTester{
addr: addr,
Expand Down Expand Up @@ -166,7 +166,7 @@ func correctBzzHandshake(addr *BzzAddr, lightNode bool) *HandshakeMsg {
func TestBzzHandshakeNetworkIDMismatch(t *testing.T) {
lightNode := false
addr := RandomAddr()
s := newBzzHandshakeTester(t, 1, addr, lightNode)
s := newBzzHandshakeTester(1, addr, lightNode)
node := s.Nodes[0]

err := s.testHandshake(
Expand All @@ -183,7 +183,7 @@ func TestBzzHandshakeNetworkIDMismatch(t *testing.T) {
func TestBzzHandshakeVersionMismatch(t *testing.T) {
lightNode := false
addr := RandomAddr()
s := newBzzHandshakeTester(t, 1, addr, lightNode)
s := newBzzHandshakeTester(1, addr, lightNode)
node := s.Nodes[0]

err := s.testHandshake(
Expand All @@ -200,7 +200,7 @@ func TestBzzHandshakeVersionMismatch(t *testing.T) {
func TestBzzHandshakeSuccess(t *testing.T) {
lightNode := false
addr := RandomAddr()
s := newBzzHandshakeTester(t, 1, addr, lightNode)
s := newBzzHandshakeTester(1, addr, lightNode)
node := s.Nodes[0]

err := s.testHandshake(
Expand All @@ -225,7 +225,8 @@ func TestBzzHandshakeLightNode(t *testing.T) {
for _, test := range lightNodeTests {
t.Run(test.name, func(t *testing.T) {
randomAddr := RandomAddr()
pt := newBzzHandshakeTester(nil, 1, randomAddr, false) // TODO change signature - t is not used anywhere
pt := newBzzHandshakeTester(1, randomAddr, false)

node := pt.Nodes[0]
addr := NewAddr(node)

Expand Down
5 changes: 2 additions & 3 deletions swarm/network/stream/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"os"
"strings"
"sync/atomic"
"testing"
"time"

"github.com/ethereum/go-ethereum/log"
Expand Down Expand Up @@ -67,7 +66,7 @@ func init() {
log.Root().SetHandler(log.LvlFilterHandler(log.Lvl(*loglevel), log.StreamHandler(colorable.NewColorableStderr(), log.TerminalFormat(true))))
}

func newStreamerTester(t *testing.T, registryOptions *RegistryOptions) (*p2ptest.ProtocolTester, *Registry, *storage.LocalStore, func(), error) {
func newStreamerTester(registryOptions *RegistryOptions) (*p2ptest.ProtocolTester, *Registry, *storage.LocalStore, func(), error) {
// setup
addr := network.RandomAddr() // tested peers peer address
to := network.NewKademlia(addr.OAddr, network.NewKadParams())
Expand Down Expand Up @@ -102,7 +101,7 @@ func newStreamerTester(t *testing.T, registryOptions *RegistryOptions) (*p2ptest
streamer.Close()
removeDataDir()
}
protocolTester := p2ptest.NewProtocolTester(t, addr.ID(), 1, streamer.runProtocol)
protocolTester := p2ptest.NewProtocolTester(addr.ID(), 1, streamer.runProtocol)

err = waitForPeers(streamer, 1*time.Second, 1)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions swarm/network/stream/delivery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestStreamerRetrieveRequest(t *testing.T) {
Retrieval: RetrievalClientOnly,
Syncing: SyncingDisabled,
}
tester, streamer, _, teardown, err := newStreamerTester(t, regOpts)
Copy link
Member

Choose a reason for hiding this comment

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

@justelad what @zelig meant here is that if you have access to testing.T inside a function, you could call inside t.Fatal in case something goes wrong, that you don't care about in the tests.

This basically helps you to get rid of all the if err != nil calls in the setup stage of tests, so that you have a more readable test.

Obviously this was not done here, but generally helper functions call t.Helper() at the top (so that they report the correct failure line), and then you don't need to return err as the test would have failed on the t.Fatal

I hope that makes sense.

I suggest we merge this PR after the linter is fixed, but this should be something we improve in the future, to make tests more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

understood. thanks for the review 🙏

tester, streamer, _, teardown, err := newStreamerTester(regOpts)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -97,7 +97,7 @@ func TestStreamerRetrieveRequest(t *testing.T) {
//Test requesting a chunk from a peer then issuing a "empty" OfferedHashesMsg (no hashes available yet)
//Should time out as the peer does not have the chunk (no syncing happened previously)
func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, &RegistryOptions{
tester, streamer, _, teardown, err := newStreamerTester(&RegistryOptions{
Retrieval: RetrievalEnabled,
Syncing: SyncingDisabled, //do no syncing
})
Expand Down Expand Up @@ -169,7 +169,7 @@ func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
// upstream request server receives a retrieve Request and responds with
// offered hashes or delivery if skipHash is set to true
func TestStreamerUpstreamRetrieveRequestMsgExchange(t *testing.T) {
tester, streamer, localStore, teardown, err := newStreamerTester(t, &RegistryOptions{
tester, streamer, localStore, teardown, err := newStreamerTester(&RegistryOptions{
Retrieval: RetrievalEnabled,
Syncing: SyncingDisabled,
})
Expand Down Expand Up @@ -359,7 +359,7 @@ func TestRequestFromPeersWithLightNode(t *testing.T) {
}

func TestStreamerDownstreamChunkDeliveryMsgExchange(t *testing.T) {
tester, streamer, localStore, teardown, err := newStreamerTester(t, &RegistryOptions{
tester, streamer, localStore, teardown, err := newStreamerTester(&RegistryOptions{
Retrieval: RetrievalDisabled,
Syncing: SyncingDisabled,
})
Expand Down
8 changes: 4 additions & 4 deletions swarm/network/stream/lightnode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestLigthnodeRetrieveRequestWithRetrieve(t *testing.T) {
Retrieval: RetrievalClientOnly,
Syncing: SyncingDisabled,
}
tester, _, _, teardown, err := newStreamerTester(t, registryOptions)
tester, _, _, teardown, err := newStreamerTester(registryOptions)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -67,7 +67,7 @@ func TestLigthnodeRetrieveRequestWithoutRetrieve(t *testing.T) {
Retrieval: RetrievalDisabled,
Syncing: SyncingDisabled,
}
tester, _, _, teardown, err := newStreamerTester(t, registryOptions)
tester, _, _, teardown, err := newStreamerTester(registryOptions)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -111,7 +111,7 @@ func TestLigthnodeRequestSubscriptionWithSync(t *testing.T) {
Retrieval: RetrievalDisabled,
Syncing: SyncingRegisterOnly,
}
tester, _, _, teardown, err := newStreamerTester(t, registryOptions)
tester, _, _, teardown, err := newStreamerTester(registryOptions)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestLigthnodeRequestSubscriptionWithoutSync(t *testing.T) {
Retrieval: RetrievalDisabled,
Syncing: SyncingDisabled,
}
tester, _, _, teardown, err := newStreamerTester(t, registryOptions)
tester, _, _, teardown, err := newStreamerTester(registryOptions)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down
26 changes: 13 additions & 13 deletions swarm/network/stream/streamer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
)

func TestStreamerSubscribe(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand All @@ -48,7 +48,7 @@ func TestStreamerSubscribe(t *testing.T) {
}

func TestStreamerRequestSubscription(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -139,7 +139,7 @@ func (self *testServer) Close() {
}

func TestStreamerDownstreamSubscribeUnsubscribeMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -232,7 +232,7 @@ func TestStreamerDownstreamSubscribeUnsubscribeMsgExchange(t *testing.T) {
}

func TestStreamerUpstreamSubscribeUnsubscribeMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -299,7 +299,7 @@ func TestStreamerUpstreamSubscribeUnsubscribeMsgExchange(t *testing.T) {
}

func TestStreamerUpstreamSubscribeUnsubscribeMsgExchangeLive(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -365,7 +365,7 @@ func TestStreamerUpstreamSubscribeUnsubscribeMsgExchangeLive(t *testing.T) {
}

func TestStreamerUpstreamSubscribeErrorMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -409,7 +409,7 @@ func TestStreamerUpstreamSubscribeErrorMsgExchange(t *testing.T) {
}

func TestStreamerUpstreamSubscribeLiveAndHistory(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -472,7 +472,7 @@ func TestStreamerUpstreamSubscribeLiveAndHistory(t *testing.T) {
}

func TestStreamerDownstreamCorruptHashesMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -537,7 +537,7 @@ func TestStreamerDownstreamCorruptHashesMsgExchange(t *testing.T) {
}

func TestStreamerDownstreamOfferedHashesMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -636,7 +636,7 @@ func TestStreamerDownstreamOfferedHashesMsgExchange(t *testing.T) {
}

func TestStreamerRequestSubscriptionQuitMsgExchange(t *testing.T) {
tester, streamer, _, teardown, err := newStreamerTester(t, nil)
tester, streamer, _, teardown, err := newStreamerTester(nil)
defer teardown()
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -769,7 +769,7 @@ func TestStreamerRequestSubscriptionQuitMsgExchange(t *testing.T) {
// leaving place for new streams.
func TestMaxPeerServersWithUnsubscribe(t *testing.T) {
var maxPeerServers = 6
tester, streamer, _, teardown, err := newStreamerTester(t, &RegistryOptions{
tester, streamer, _, teardown, err := newStreamerTester(&RegistryOptions{
Retrieval: RetrievalDisabled,
Syncing: SyncingDisabled,
MaxPeerServers: maxPeerServers,
Expand Down Expand Up @@ -845,7 +845,7 @@ func TestMaxPeerServersWithUnsubscribe(t *testing.T) {
// error message exchange.
func TestMaxPeerServersWithoutUnsubscribe(t *testing.T) {
var maxPeerServers = 6
tester, streamer, _, teardown, err := newStreamerTester(t, &RegistryOptions{
tester, streamer, _, teardown, err := newStreamerTester(&RegistryOptions{
MaxPeerServers: maxPeerServers,
})
defer teardown()
Expand Down Expand Up @@ -930,7 +930,7 @@ func TestMaxPeerServersWithoutUnsubscribe(t *testing.T) {
//TestHasPriceImplementation is to check that the Registry has a
//`Price` interface implementation
func TestHasPriceImplementation(t *testing.T) {
_, r, _, teardown, err := newStreamerTester(t, &RegistryOptions{
_, r, _, teardown, err := newStreamerTester(&RegistryOptions{
Retrieval: RetrievalDisabled,
Syncing: SyncingDisabled,
})
Expand Down