From 75f3390583d9d2c2d6950dd28072ad711cfdd3d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Fri, 19 Nov 2021 15:48:57 +0000 Subject: [PATCH] mark all test helper funcs via t.Helper This will allow testing errors to point to the caller position, which is more useful than where the test helper is declared. While here, use testing.TB for its intended purpose. That interface is broader than TestingT, but we're also never going to use an implementation other than testing. --- allocator/allocator_test.go | 1 + impl/graphsync_test.go | 1 + ipldutil/traverser_test.go | 1 + .../asyncloader/asyncloader_test.go | 2 ++ requestmanager/requestmanager_test.go | 2 ++ requestmanager/testloader/asyncloader.go | 4 +++ .../queryexecutor/queryexecutor_test.go | 2 ++ .../responseassembler_test.go | 4 +++ responsemanager/responsemanager_test.go | 1 + testutil/channelassertions.go | 19 ++++++++---- testutil/testchain.go | 13 ++------ testutil/testconnmanager.go | 13 +++++--- testutil/testnotifications.go | 6 ++++ testutil/testutil.go | 31 +++++++++++++------ 14 files changed, 70 insertions(+), 30 deletions(-) diff --git a/allocator/allocator_test.go b/allocator/allocator_test.go index 6487480a..c4ca4661 100644 --- a/allocator/allocator_test.go +++ b/allocator/allocator_test.go @@ -395,6 +395,7 @@ func TestAllocator(t *testing.T) { } func readPending(t *testing.T, pending []pendingResultWithChan) []pendingResultWithChan { + t.Helper() morePending := true for morePending && len(pending) > 0 { morePending = false diff --git a/impl/graphsync_test.go b/impl/graphsync_test.go index 2ea53e67..6d0a9ce1 100644 --- a/impl/graphsync_test.go +++ b/impl/graphsync_test.go @@ -1145,6 +1145,7 @@ type gsTestData struct { } func newGsTestData(ctx context.Context, t *testing.T) *gsTestData { + t.Helper() td := &gsTestData{ctx: ctx} td.mn = mocknet.New(ctx) var err error diff --git a/ipldutil/traverser_test.go b/ipldutil/traverser_test.go index f19af589..ac30612f 100644 --- a/ipldutil/traverser_test.go +++ b/ipldutil/traverser_test.go @@ -140,6 +140,7 @@ func TestTraverser(t *testing.T) { } func checkTraverseSequence(ctx context.Context, t *testing.T, traverser Traverser, expectedBlks []blocks.Block, finalErr error) { + t.Helper() for _, blk := range expectedBlks { isComplete, err := traverser.IsComplete() require.False(t, isComplete) diff --git a/requestmanager/asyncloader/asyncloader_test.go b/requestmanager/asyncloader/asyncloader_test.go index 941554a6..fcde371c 100644 --- a/requestmanager/asyncloader/asyncloader_test.go +++ b/requestmanager/asyncloader/asyncloader_test.go @@ -391,6 +391,7 @@ func withLoader(st *store, exec func(ctx context.Context, asyncLoader *AsyncLoad } func assertSuccessResponse(ctx context.Context, t *testing.T, resultChan <-chan types.AsyncLoadResult) { + t.Helper() var result types.AsyncLoadResult testutil.AssertReceive(ctx, t, resultChan, &result, "should close response channel with response") require.NotNil(t, result.Data, "should send response") @@ -398,6 +399,7 @@ func assertSuccessResponse(ctx context.Context, t *testing.T, resultChan <-chan } func assertFailResponse(ctx context.Context, t *testing.T, resultChan <-chan types.AsyncLoadResult) { + t.Helper() var result types.AsyncLoadResult testutil.AssertReceive(ctx, t, resultChan, &result, "should close response channel with response") require.Nil(t, result.Data, "should not send responses") diff --git a/requestmanager/requestmanager_test.go b/requestmanager/requestmanager_test.go index 3a601c80..44e7dc4e 100644 --- a/requestmanager/requestmanager_test.go +++ b/requestmanager/requestmanager_test.go @@ -1031,6 +1031,7 @@ func metadataForBlocks(blks []blocks.Block, present bool) metadata.Metadata { } func encodedMetadataForBlocks(t *testing.T, blks []blocks.Block, present bool) graphsync.ExtensionData { + t.Helper() md := metadataForBlocks(blks, present) metadataEncoded, err := metadata.EncodeMetadata(md) require.NoError(t, err, "did not encode metadata") @@ -1065,6 +1066,7 @@ type testData struct { } func newTestData(ctx context.Context, t *testing.T) *testData { + t.Helper() td := &testData{} td.requestRecordChan = make(chan requestRecord, 3) td.fph = &fakePeerHandler{td.requestRecordChan} diff --git a/requestmanager/testloader/asyncloader.go b/requestmanager/testloader/asyncloader.go index 3d896ed8..97f01c77 100644 --- a/requestmanager/testloader/asyncloader.go +++ b/requestmanager/testloader/asyncloader.go @@ -70,6 +70,7 @@ func (fal *FakeAsyncLoader) ProcessResponse(responses map[graphsync.RequestID]me // VerifyLastProcessedBlocks verifies the blocks passed to the last call to ProcessResponse // match the expected ones func (fal *FakeAsyncLoader) VerifyLastProcessedBlocks(ctx context.Context, t *testing.T, expectedBlocks []blocks.Block) { + t.Helper() var processedBlocks []blocks.Block testutil.AssertReceive(ctx, t, fal.blks, &processedBlocks, "did not process blocks") require.Equal(t, expectedBlocks, processedBlocks, "did not process correct blocks") @@ -79,6 +80,7 @@ func (fal *FakeAsyncLoader) VerifyLastProcessedBlocks(ctx context.Context, t *te // match the expected ones func (fal *FakeAsyncLoader) VerifyLastProcessedResponses(ctx context.Context, t *testing.T, expectedResponses map[graphsync.RequestID]metadata.Metadata) { + t.Helper() var responses map[graphsync.RequestID]metadata.Metadata testutil.AssertReceive(ctx, t, fal.responses, &responses, "did not process responses") require.Equal(t, expectedResponses, responses, "did not process correct responses") @@ -87,6 +89,7 @@ func (fal *FakeAsyncLoader) VerifyLastProcessedResponses(ctx context.Context, t // VerifyNoRemainingData verifies no outstanding response channels are open for the given // RequestID (CleanupRequest was called last) func (fal *FakeAsyncLoader) VerifyNoRemainingData(t *testing.T, requestID graphsync.RequestID) { + t.Helper() fal.responseChannelsLk.RLock() for key := range fal.responseChannels { require.NotEqual(t, key.requestID, requestID, "did not clean up request properly") @@ -96,6 +99,7 @@ func (fal *FakeAsyncLoader) VerifyNoRemainingData(t *testing.T, requestID graphs // VerifyStoreUsed verifies the given store was used for the given request func (fal *FakeAsyncLoader) VerifyStoreUsed(t *testing.T, requestID graphsync.RequestID, storeName string) { + t.Helper() fal.storesRequestedLk.RLock() _, ok := fal.storesRequested[storeKey{requestID, storeName}] require.True(t, ok, "request should load from correct store") diff --git a/responsemanager/queryexecutor/queryexecutor_test.go b/responsemanager/queryexecutor/queryexecutor_test.go index dd1e0a94..50a1b697 100644 --- a/responsemanager/queryexecutor/queryexecutor_test.go +++ b/responsemanager/queryexecutor/queryexecutor_test.go @@ -243,6 +243,7 @@ func TestSmallGraphTask(t *testing.T) { } func notifeeExpect(t *testing.T, td *testData, expectedCalls int, expectedFinalData notifications.TopicData) { + t.Helper() notifeeCount := 1 td.responseBuilder.notifeeCb = func(n notifications.Notifee) { require.Same(t, td.subscriber, n.Subscriber) @@ -307,6 +308,7 @@ type testData struct { } func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, *QueryExecutor) { + t.Helper() ctx := context.Background() td := &testData{} td.t = t diff --git a/responsemanager/responseassembler/responseassembler_test.go b/responsemanager/responseassembler/responseassembler_test.go index 6ce8710c..b13cbd15 100644 --- a/responsemanager/responseassembler/responseassembler_test.go +++ b/responsemanager/responseassembler/responseassembler_test.go @@ -424,18 +424,21 @@ func findResponseForRequestID(responses []gsmsg.GraphSyncResponse, requestID gra } func assertSentNotOnWire(t *testing.T, bd graphsync.BlockData, blk blocks.Block) { + t.Helper() require.Equal(t, cidlink.Link{Cid: blk.Cid()}, bd.Link()) require.Equal(t, uint64(len(blk.RawData())), bd.BlockSize()) require.Equal(t, uint64(0), bd.BlockSizeOnWire()) } func assertSentOnWire(t *testing.T, bd graphsync.BlockData, blk blocks.Block) { + t.Helper() require.Equal(t, cidlink.Link{Cid: blk.Cid()}, bd.Link()) require.Equal(t, uint64(len(blk.RawData())), bd.BlockSize()) require.Equal(t, uint64(len(blk.RawData())), bd.BlockSizeOnWire()) } func assertNotSent(t *testing.T, bd graphsync.BlockData, blk blocks.Block) { + t.Helper() require.Equal(t, cidlink.Link{Cid: blk.Cid()}, bd.Link()) require.Equal(t, uint64(0), bd.BlockSize()) require.Equal(t, uint64(0), bd.BlockSizeOnWire()) @@ -451,6 +454,7 @@ type fakePeerHandler struct { } func newFakePeerHandler(ctx context.Context, t *testing.T) *fakePeerHandler { + t.Helper() return &fakePeerHandler{ ctx: ctx, t: t, diff --git a/responsemanager/responsemanager_test.go b/responsemanager/responsemanager_test.go index d8ad4f8a..b05582d4 100644 --- a/responsemanager/responsemanager_test.go +++ b/responsemanager/responsemanager_test.go @@ -989,6 +989,7 @@ type testData struct { } func newTestData(t *testing.T) testData { + t.Helper() ctx := context.Background() td := testData{} td.t = t diff --git a/testutil/channelassertions.go b/testutil/channelassertions.go index 58c84e66..b809f94f 100644 --- a/testutil/channelassertions.go +++ b/testutil/channelassertions.go @@ -3,19 +3,22 @@ package testutil import ( "context" "reflect" + "testing" "github.com/stretchr/testify/require" ) // AssertReceive verifies that a channel returns a value before the given context closes, and writes into // into out, which should be a pointer to the value type -func AssertReceive(ctx context.Context, t TestingT, channel interface{}, out interface{}, errorMessage string) { +func AssertReceive(ctx context.Context, t testing.TB, channel interface{}, out interface{}, errorMessage string) { + t.Helper() AssertReceiveFirst(t, channel, out, errorMessage, ctx.Done()) } // AssertReceiveFirst verifies that a channel returns a value on the specified channel before the other channels, // and writes the value into out, which should be a pointer to the value type -func AssertReceiveFirst(t TestingT, channel interface{}, out interface{}, errorMessage string, incorrectChannels ...interface{}) { +func AssertReceiveFirst(t testing.TB, channel interface{}, out interface{}, errorMessage string, incorrectChannels ...interface{}) { + t.Helper() chanValue := reflect.ValueOf(channel) outValue := reflect.ValueOf(out) require.Equal(t, reflect.Chan, chanValue.Kind(), "incorrect argument: should pass channel to read from") @@ -44,12 +47,14 @@ func AssertReceiveFirst(t TestingT, channel interface{}, out interface{}, errorM } // AssertDoesReceive verifies that a channel returns some value before the given context closes -func AssertDoesReceive(ctx context.Context, t TestingT, channel interface{}, errorMessage string) { +func AssertDoesReceive(ctx context.Context, t testing.TB, channel interface{}, errorMessage string) { + t.Helper() AssertDoesReceiveFirst(t, channel, errorMessage, ctx.Done()) } // AssertDoesReceiveFirst asserts that the given channel receives a value before any of the other channels specified -func AssertDoesReceiveFirst(t TestingT, channel interface{}, errorMessage string, incorrectChannels ...interface{}) { +func AssertDoesReceiveFirst(t testing.TB, channel interface{}, errorMessage string, incorrectChannels ...interface{}) { + t.Helper() chanValue := reflect.ValueOf(channel) require.Equal(t, reflect.Chan, chanValue.Kind(), "incorrect argument: should pass channel to read from") require.Contains(t, []reflect.ChanDir{reflect.BothDir, reflect.RecvDir}, chanValue.Type().ChanDir(), "incorrect argument: should pass a receiving channel") @@ -73,7 +78,8 @@ func AssertDoesReceiveFirst(t TestingT, channel interface{}, errorMessage string } // AssertChannelEmpty verifies that a channel has no value currently -func AssertChannelEmpty(t TestingT, channel interface{}, errorMessage string) { +func AssertChannelEmpty(t testing.TB, channel interface{}, errorMessage string) { + t.Helper() chanValue := reflect.ValueOf(channel) require.Equal(t, reflect.Chan, chanValue.Kind(), "incorrect argument: should pass channel to read from") require.Contains(t, []reflect.ChanDir{reflect.BothDir, reflect.RecvDir}, chanValue.Type().ChanDir(), "incorrect argument: should pass a receiving channel") @@ -90,7 +96,8 @@ func AssertChannelEmpty(t TestingT, channel interface{}, errorMessage string) { } // AssertSends attempts to send the given input value to the given channel before the given context closes -func AssertSends(ctx context.Context, t TestingT, channel interface{}, in interface{}, errorMessage string) { +func AssertSends(ctx context.Context, t testing.TB, channel interface{}, in interface{}, errorMessage string) { + t.Helper() chanValue := reflect.ValueOf(channel) inValue := reflect.ValueOf(in) require.Equal(t, reflect.Chan, chanValue.Kind(), "incorrect argument: should pass channel to send to") diff --git a/testutil/testchain.go b/testutil/testchain.go index 5998446a..5d00ae4a 100644 --- a/testutil/testchain.go +++ b/testutil/testchain.go @@ -3,6 +3,7 @@ package testutil import ( "context" "io/ioutil" + "testing" blocks "github.com/ipfs/go-block-format" cid "github.com/ipfs/go-cid" @@ -18,20 +19,12 @@ import ( "github.com/ipfs/go-graphsync/testutil/chaintypes" ) -// TestingT covers the interface methods we need from either *testing.T or -// *testing.B -type TestingT interface { - Errorf(format string, args ...interface{}) - FailNow() - Fatal(args ...interface{}) -} - const blockChainTraversedNodesPerBlock = 2 // TestBlockChain is a simulated data structure similar to a blockchain // which graphsync is uniquely suited for type TestBlockChain struct { - t TestingT + t testing.TB blockChainLength int loader ipld.BlockReadOpener GenisisNode ipld.Node @@ -95,7 +88,7 @@ func createBlock(parents []ipld.Link, size uint64) (ipld.Node, error) { // SetupBlockChain creates a new test block chain with the given height func SetupBlockChain( ctx context.Context, - t TestingT, + t testing.TB, lsys ipld.LinkSystem, size uint64, blockChainLength int) *TestBlockChain { diff --git a/testutil/testconnmanager.go b/testutil/testconnmanager.go index ad9f9c15..8451a707 100644 --- a/testutil/testconnmanager.go +++ b/testutil/testconnmanager.go @@ -2,6 +2,7 @@ package testutil import ( "sync" + "testing" "github.com/libp2p/go-libp2p-core/peer" "github.com/stretchr/testify/require" @@ -45,14 +46,16 @@ func (tcm *TestConnManager) Unprotect(p peer.ID, tag string) bool { } // AssertProtected asserts that the connection is protected by at least one tag -func (tcm *TestConnManager) AssertProtected(t TestingT, p peer.ID) { +func (tcm *TestConnManager) AssertProtected(t testing.TB, p peer.ID) { + t.Helper() tcm.protectedConnsLk.RLock() defer tcm.protectedConnsLk.RUnlock() require.True(t, len(tcm.protectedConns[p]) > 0) } // RefuteProtected refutes that a connection has been protect -func (tcm *TestConnManager) RefuteProtected(t TestingT, p peer.ID) { +func (tcm *TestConnManager) RefuteProtected(t testing.TB, p peer.ID) { + t.Helper() tcm.protectedConnsLk.RLock() defer tcm.protectedConnsLk.RUnlock() require.False(t, len(tcm.protectedConns[p]) > 0) @@ -60,7 +63,8 @@ func (tcm *TestConnManager) RefuteProtected(t TestingT, p peer.ID) { // AssertProtectedWithTags verifies the connection is protected with the given // tags at least -func (tcm *TestConnManager) AssertProtectedWithTags(t TestingT, p peer.ID, tags ...string) { +func (tcm *TestConnManager) AssertProtectedWithTags(t testing.TB, p peer.ID, tags ...string) { + t.Helper() tcm.protectedConnsLk.RLock() defer tcm.protectedConnsLk.RUnlock() for _, tag := range tags { @@ -70,7 +74,8 @@ func (tcm *TestConnManager) AssertProtectedWithTags(t TestingT, p peer.ID, tags // RefuteProtectedWithTags verifies the connection is not protected with any of the given // tags -func (tcm *TestConnManager) RefuteProtectedWithTags(t TestingT, p peer.ID, tags ...string) { +func (tcm *TestConnManager) RefuteProtectedWithTags(t testing.TB, p peer.ID, tags ...string) { + t.Helper() tcm.protectedConnsLk.RLock() defer tcm.protectedConnsLk.RUnlock() for _, tag := range tags { diff --git a/testutil/testnotifications.go b/testutil/testnotifications.go index 53e7ce60..f6709dfa 100644 --- a/testutil/testnotifications.go +++ b/testutil/testnotifications.go @@ -36,6 +36,7 @@ func (ts *TestSubscriber) OnClose(topic notifications.Topic) { } func (ts *TestSubscriber) ExpectEvents(ctx context.Context, t *testing.T, events []DispatchedEvent) { + t.Helper() for _, expectedEvent := range events { var event DispatchedEvent AssertReceive(ctx, t, ts.receivedEvents, &event, "should receive another event") @@ -44,10 +45,12 @@ func (ts *TestSubscriber) ExpectEvents(ctx context.Context, t *testing.T, events } func (ts *TestSubscriber) NoEventsReceived(t *testing.T) { + t.Helper() AssertChannelEmpty(t, ts.receivedEvents, "should have received no events") } func (ts *TestSubscriber) ExpectClosesAnyOrder(ctx context.Context, t *testing.T, topics []notifications.Topic) { + t.Helper() expectedTopics := make(map[notifications.Topic]struct{}) receivedTopics := make(map[notifications.Topic]struct{}) for _, expectedTopic := range topics { @@ -60,6 +63,7 @@ func (ts *TestSubscriber) ExpectClosesAnyOrder(ctx context.Context, t *testing.T } func (ts *TestSubscriber) ExpectCloses(ctx context.Context, t *testing.T, topics []notifications.Topic) { + t.Helper() for _, expectedTopic := range topics { var topic notifications.Topic AssertReceive(ctx, t, ts.closed, &topic, "should receive another event") @@ -73,6 +77,7 @@ type NotifeeVerifier struct { } func (nv *NotifeeVerifier) ExpectEvents(ctx context.Context, t *testing.T, events []notifications.Event) { + t.Helper() dispatchedEvents := make([]DispatchedEvent, 0, len(events)) for _, ev := range events { dispatchedEvents = append(dispatchedEvents, DispatchedEvent{nv.expectedTopic, ev}) @@ -81,6 +86,7 @@ func (nv *NotifeeVerifier) ExpectEvents(ctx context.Context, t *testing.T, event } func (nv *NotifeeVerifier) ExpectClose(ctx context.Context, t *testing.T) { + t.Helper() nv.subscriber.ExpectCloses(ctx, t, []notifications.Topic{nv.expectedTopic}) } diff --git a/testutil/testutil.go b/testutil/testutil.go index 6fdb7005..2692fb46 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -79,12 +79,14 @@ func ContainsPeer(peers []peer.ID, p peer.ID) bool { } // AssertContainsPeer will fail a test if the peer is not in the given peer list -func AssertContainsPeer(t TestingT, peers []peer.ID, p peer.ID) { +func AssertContainsPeer(t testing.TB, peers []peer.ID, p peer.ID) { + t.Helper() require.True(t, ContainsPeer(peers, p), "given peer should be in list") } // RefuteContainsPeer will fail a test if the peer is in the given peer list -func RefuteContainsPeer(t TestingT, peers []peer.ID, p peer.ID) { +func RefuteContainsPeer(t testing.TB, peers []peer.ID, p peer.ID) { + t.Helper() require.False(t, ContainsPeer(peers, p), "given peer should not be in list") } @@ -104,18 +106,21 @@ func ContainsBlock(blks []blocks.Block, block blocks.Block) bool { } // AssertContainsBlock will fail a test if the block is not in the given block list -func AssertContainsBlock(t TestingT, blks []blocks.Block, block blocks.Block) { +func AssertContainsBlock(t testing.TB, blks []blocks.Block, block blocks.Block) { + t.Helper() require.True(t, ContainsBlock(blks, block), "given block should be in list") } // RefuteContainsBlock will fail a test if the block is in the given block list -func RefuteContainsBlock(t TestingT, blks []blocks.Block, block blocks.Block) { +func RefuteContainsBlock(t testing.TB, blks []blocks.Block, block blocks.Block) { + t.Helper() require.False(t, ContainsBlock(blks, block), "given block should not be in list") } // CollectResponses is just a utility to convert a graphsync response progress // channel into an array. -func CollectResponses(ctx context.Context, t TestingT, responseChan <-chan graphsync.ResponseProgress) []graphsync.ResponseProgress { +func CollectResponses(ctx context.Context, t testing.TB, responseChan <-chan graphsync.ResponseProgress) []graphsync.ResponseProgress { + t.Helper() var collectedBlocks []graphsync.ResponseProgress for { select { @@ -132,6 +137,7 @@ func CollectResponses(ctx context.Context, t TestingT, responseChan <-chan graph // CollectErrors is just a utility to convert an error channel into an array. func CollectErrors(ctx context.Context, t *testing.T, errChan <-chan error) []error { + t.Helper() var collectedErrors []error for { select { @@ -148,7 +154,8 @@ func CollectErrors(ctx context.Context, t *testing.T, errChan <-chan error) []er // ReadNResponses does a partial read from a ResponseProgress channel -- up // to n values -func ReadNResponses(ctx context.Context, t TestingT, responseChan <-chan graphsync.ResponseProgress, count int) []graphsync.ResponseProgress { +func ReadNResponses(ctx context.Context, t testing.TB, responseChan <-chan graphsync.ResponseProgress, count int) []graphsync.ResponseProgress { + t.Helper() var returnedBlocks []graphsync.ResponseProgress for i := 0; i < count; i++ { select { @@ -166,7 +173,8 @@ func ReadNResponses(ctx context.Context, t TestingT, responseChan <-chan graphsy // VerifySingleTerminalError verifies that exactly one error was sent over a channel // and then the channel was closed. -func VerifySingleTerminalError(ctx context.Context, t TestingT, errChan <-chan error) { +func VerifySingleTerminalError(ctx context.Context, t testing.TB, errChan <-chan error) { + t.Helper() var err error AssertReceive(ctx, t, errChan, &err, "should receive an error") select { @@ -178,7 +186,8 @@ func VerifySingleTerminalError(ctx context.Context, t TestingT, errChan <-chan e } // VerifyHasErrors verifies that at least one error was sent over a channel -func VerifyHasErrors(ctx context.Context, t TestingT, errChan <-chan error) { +func VerifyHasErrors(ctx context.Context, t testing.TB, errChan <-chan error) { + t.Helper() errCount := 0 for { select { @@ -196,7 +205,8 @@ func VerifyHasErrors(ctx context.Context, t TestingT, errChan <-chan error) { // VerifyEmptyErrors verifies that no errors were sent over a channel before // it was closed -func VerifyEmptyErrors(ctx context.Context, t TestingT, errChan <-chan error) { +func VerifyEmptyErrors(ctx context.Context, t testing.TB, errChan <-chan error) { + t.Helper() for { select { case _, ok := <-errChan: @@ -212,7 +222,8 @@ func VerifyEmptyErrors(ctx context.Context, t TestingT, errChan <-chan error) { // VerifyEmptyResponse verifies that no response progress happened before the // channel was closed. -func VerifyEmptyResponse(ctx context.Context, t TestingT, responseChan <-chan graphsync.ResponseProgress) { +func VerifyEmptyResponse(ctx context.Context, t testing.TB, responseChan <-chan graphsync.ResponseProgress) { + t.Helper() for { select { case _, ok := <-responseChan: