[chore]: enable len and empty rules from testifylint (open-telemetry#…

…11021) #### Description Testifylint is a linter that provides best practices with the use of testify. This PR enables [len](https://github.com/Antonboom/testifylint?tab=readme-ov-file#len) and [empty](https://github.com/Antonboom/testifylint?tab=readme-ov-file#empty) rules from [testifylint](https://github.com/Antonboom/testifylint) It also adds testifylint as tool to use with a make command Signed-off-by: Matthieu MOREL <[email protected]>
dmathieu · Sep 10, 2024 · bb6b94d · bb6b94d
1 parent cba695e
commit bb6b94d
Show file tree

Hide file tree

Showing 37 changed files with 130 additions and 118 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -125,14 +125,12 @@ linters-settings:
     # TODO: enable all rules
     disable:
       - compares
-      - empty
       - error-is-as
       - error-nil
       - expected-actual
       - float-compare
       - formatter
       - go-require
-      - len
       - negative-positive
       - nil-compare
       - require-error

diff --git a/Makefile b/Makefile
@@ -56,6 +56,10 @@ gotest-with-cover:
 	@$(MAKE) for-all-target TARGET="test-with-cover"
 	$(GOCMD) tool covdata textfmt -i=./coverage/unit -o ./coverage.txt
 
+.PHONY: gotestifylint-fix
+gotestifylint-fix:
+	$(MAKE) for-all-target TARGET="testifylint-fix"
+
 .PHONY: goporto
 goporto: $(PORTO)
 	$(PORTO) -w --include-internal --skip-dirs "^cmd/mdatagen/third_party$$" ./

diff --git a/Makefile.Common b/Makefile.Common
@@ -36,6 +36,9 @@ MULTIMOD     := $(TOOLS_BIN_DIR)/multimod
 PORTO        := $(TOOLS_BIN_DIR)/porto
 SEMCONVGEN   := $(TOOLS_BIN_DIR)/semconvgen
 SEMCONVKIT   := $(TOOLS_BIN_DIR)/semconvkit
+TESTIFYLINT  := $(TOOLS_BIN_DIR)/testifylint
+
+TESTIFYLINT_OPT?= --enable-all --disable=compares,error-is-as,error-nil,expected-actual,float-compare,formatter,go-require,negative-positive,nil-compare,require-error
 
 .PHONY: install-tools
 install-tools: $(TOOLS_BIN_NAMES)
@@ -88,3 +91,9 @@ impi: $(IMPI)
 .PHONY: moddownload
 moddownload:
 	$(GOCMD) mod download
+
+.PHONY: testifylint-fix
+testifylint-fix:$(TESTIFYLINT)
+	@echo "running $(TESTIFYLINT)"
+	@$(TESTIFYLINT) $(TESTIFYLINT_OPT) -fix ./...
+
diff --git a/cmd/builder/internal/builder/config_test.go b/cmd/builder/internal/builder/config_test.go
@@ -171,10 +171,10 @@ func TestNewBuiltinConfig(t *testing.T) {
 	// Unlike the config initialized in NewDefaultConfig(), we expect
 	// the builtin default to be practically useful, so there must be
 	// a set of modules present.
-	assert.NotZero(t, len(cfg.Receivers))
-	assert.NotZero(t, len(cfg.Exporters))
-	assert.NotZero(t, len(cfg.Extensions))
-	assert.NotZero(t, len(cfg.Processors))
+	assert.NotEmpty(t, cfg.Receivers)
+	assert.NotEmpty(t, cfg.Exporters)
+	assert.NotEmpty(t, cfg.Extensions)
+	assert.NotEmpty(t, cfg.Processors)
 }
 
 func TestSkipGoValidation(t *testing.T) {

diff --git a/connector/connectorprofiles/profiles_router_test.go b/connector/connectorprofiles/profiles_router_test.go
@@ -120,8 +120,8 @@ func TestProfilessRouterConsumer(t *testing.T) {
 	assert.Len(t, rcs, 2)
 	assert.ElementsMatch(t, []component.ID{fooID, barID}, rcs)
 
-	assert.Len(t, foo.AllProfiles(), 0)
-	assert.Len(t, bar.AllProfiles(), 0)
+	assert.Empty(t, foo.AllProfiles())
+	assert.Empty(t, bar.AllProfiles())
 
 	both, err := r.Consumer(fooID, barID)
 	assert.NotNil(t, both)

diff --git a/connector/forwardconnector/forward_test.go b/connector/forwardconnector/forward_test.go
@@ -57,7 +57,7 @@ func TestForward(t *testing.T) {
 	assert.NoError(t, metricsToMetrics.Shutdown(ctx))
 	assert.NoError(t, logsToLogs.Shutdown(ctx))
 
-	assert.Equal(t, 1, len(tracesSink.AllTraces()))
-	assert.Equal(t, 2, len(metricsSink.AllMetrics()))
-	assert.Equal(t, 3, len(logsSink.AllLogs()))
+	assert.Len(t, tracesSink.AllTraces(), 1)
+	assert.Len(t, metricsSink.AllMetrics(), 2)
+	assert.Len(t, logsSink.AllLogs(), 3)
 }
diff --git a/connector/logs_router_test.go b/connector/logs_router_test.go
@@ -119,8 +119,8 @@ func TestLogsRouterConsumers(t *testing.T) {
 	assert.Len(t, rcs, 2)
 	assert.ElementsMatch(t, []component.ID{fooID, barID}, rcs)
 
-	assert.Len(t, foo.AllLogs(), 0)
-	assert.Len(t, bar.AllLogs(), 0)
+	assert.Empty(t, foo.AllLogs())
+	assert.Empty(t, bar.AllLogs())
 
 	both, err := r.Consumer(fooID, barID)
 	assert.NotNil(t, both)

diff --git a/connector/metrics_router_test.go b/connector/metrics_router_test.go
@@ -119,8 +119,8 @@ func TestMetricsRouterConsumers(t *testing.T) {
 	assert.Len(t, rcs, 2)
 	assert.ElementsMatch(t, []component.ID{fooID, barID}, rcs)
 
-	assert.Len(t, foo.AllMetrics(), 0)
-	assert.Len(t, bar.AllMetrics(), 0)
+	assert.Empty(t, foo.AllMetrics())
+	assert.Empty(t, bar.AllMetrics())
 
 	both, err := r.Consumer(fooID, barID)
 	assert.NotNil(t, both)

diff --git a/connector/traces_router_test.go b/connector/traces_router_test.go
@@ -119,8 +119,8 @@ func TestTracesRouterConsumer(t *testing.T) {
 	assert.Len(t, rcs, 2)
 	assert.ElementsMatch(t, []component.ID{fooID, barID}, rcs)
 
-	assert.Len(t, foo.AllTraces(), 0)
-	assert.Len(t, bar.AllTraces(), 0)
+	assert.Empty(t, foo.AllTraces())
+	assert.Empty(t, bar.AllTraces())
 
 	both, err := r.Consumer(fooID, barID)
 	assert.NotNil(t, both)

diff --git a/consumer/consumertest/sink_test.go b/consumer/consumertest/sink_test.go
@@ -28,7 +28,7 @@ func TestTracesSink(t *testing.T) {
 	assert.Equal(t, want, sink.AllTraces())
 	assert.Equal(t, len(want), sink.SpanCount())
 	sink.Reset()
-	assert.Equal(t, 0, len(sink.AllTraces()))
+	assert.Empty(t, sink.AllTraces())
 	assert.Equal(t, 0, sink.SpanCount())
 }
 
@@ -43,7 +43,7 @@ func TestMetricsSink(t *testing.T) {
 	assert.Equal(t, want, sink.AllMetrics())
 	assert.Equal(t, 2*len(want), sink.DataPointCount())
 	sink.Reset()
-	assert.Equal(t, 0, len(sink.AllMetrics()))
+	assert.Empty(t, sink.AllMetrics())
 	assert.Equal(t, 0, sink.DataPointCount())
 }
 
@@ -58,7 +58,7 @@ func TestLogsSink(t *testing.T) {
 	assert.Equal(t, want, sink.AllLogs())
 	assert.Equal(t, len(want), sink.LogRecordCount())
 	sink.Reset()
-	assert.Equal(t, 0, len(sink.AllLogs()))
+	assert.Empty(t, sink.AllLogs())
 	assert.Equal(t, 0, sink.LogRecordCount())
 }
 
@@ -72,5 +72,5 @@ func TestProfilesSink(t *testing.T) {
 	}
 	assert.Equal(t, want, sink.AllProfiles())
 	sink.Reset()
-	assert.Equal(t, 0, len(sink.AllProfiles()))
+	assert.Empty(t, sink.AllProfiles())
 }
diff --git a/exporter/exporterhelper/logs_test.go b/exporter/exporterhelper/logs_test.go
@@ -418,7 +418,7 @@ func checkWrapSpanForLogsExporter(t *testing.T, sr *tracetest.SpanRecorder, trac
 
 	// Inspection time!
 	gotSpanData := sr.Ended()
-	require.Equal(t, numRequests+1, len(gotSpanData))
+	require.Len(t, gotSpanData, numRequests+1)
 
 	parentSpan := gotSpanData[numRequests]
 	require.Equalf(t, fakeLogsParentSpanName, parentSpan.Name(), "SpanData %v", parentSpan)

diff --git a/exporter/exporterhelper/metrics_test.go b/exporter/exporterhelper/metrics_test.go
@@ -424,7 +424,7 @@ func checkWrapSpanForMetricsExporter(t *testing.T, sr *tracetest.SpanRecorder, t
 
 	// Inspection time!
 	gotSpanData := sr.Ended()
-	require.Equal(t, numRequests+1, len(gotSpanData))
+	require.Len(t, gotSpanData, numRequests+1)
 
 	parentSpan := gotSpanData[numRequests]
 	require.Equalf(t, fakeMetricsParentSpanName, parentSpan.Name(), "SpanData %v", parentSpan)

diff --git a/exporter/exporterhelper/traces_test.go b/exporter/exporterhelper/traces_test.go
@@ -426,7 +426,7 @@ func checkWrapSpanForTracesExporter(t *testing.T, sr *tracetest.SpanRecorder, tr
 
 	// Inspection time!
 	gotSpanData := sr.Ended()
-	require.Equal(t, numRequests+1, len(gotSpanData))
+	require.Len(t, gotSpanData, numRequests+1)
 
 	parentSpan := gotSpanData[numRequests]
 	require.Equalf(t, fakeTraceParentSpanName, parentSpan.Name(), "SpanData %v", parentSpan)

diff --git a/exporter/otlpexporter/otlp_test.go b/exporter/otlpexporter/otlp_test.go
@@ -300,7 +300,7 @@ func TestSendTraces(t *testing.T) {
 
 	md := rcv.getMetadata()
 	require.EqualValues(t, md.Get("header"), expectedHeader)
-	require.Equal(t, len(md.Get("User-Agent")), 1)
+	require.Len(t, md.Get("User-Agent"), 1)
 	require.Contains(t, md.Get("User-Agent")[0], "Collector/1.2.3test")
 
 	// Return partial success
@@ -472,7 +472,7 @@ func TestSendMetrics(t *testing.T) {
 
 	mdata := rcv.getMetadata()
 	require.EqualValues(t, mdata.Get("header"), expectedHeader)
-	require.Equal(t, len(mdata.Get("User-Agent")), 1)
+	require.Len(t, mdata.Get("User-Agent"), 1)
 	require.Contains(t, mdata.Get("User-Agent")[0], "Collector/1.2.3test")
 
 	st := status.New(codes.InvalidArgument, "Invalid argument")
@@ -763,7 +763,7 @@ func TestSendLogData(t *testing.T) {
 	assert.EqualValues(t, ld, rcv.getLastRequest())
 
 	md := rcv.getMetadata()
-	require.Equal(t, len(md.Get("User-Agent")), 1)
+	require.Len(t, md.Get("User-Agent"), 1)
 	require.Contains(t, md.Get("User-Agent")[0], "Collector/1.2.3test")
 
 	st := status.New(codes.InvalidArgument, "Invalid argument")

diff --git a/internal/e2e/status_test.go b/internal/e2e/status_test.go
@@ -121,7 +121,7 @@ func Test_ComponentStatusReporting_SharedInstance(t *testing.T) {
 	err = s.Shutdown(context.Background())
 	require.NoError(t, err)
 
-	require.Equal(t, 2, len(eventsReceived))
+	require.Len(t, eventsReceived, 2)
 
 	for instanceID, events := range eventsReceived {
 		pipelineIDs := ""

diff --git a/internal/sharedcomponent/sharedcomponent_test.go b/internal/sharedcomponent/sharedcomponent_test.go
@@ -26,20 +26,20 @@ type baseComponent struct {
 
 func TestNewMap(t *testing.T) {
 	comps := NewMap[component.ID, *baseComponent]()
-	assert.Len(t, comps.components, 0)
+	assert.Empty(t, comps.components)
 }
 
 func TestNewSharedComponentsCreateError(t *testing.T) {
 	comps := NewMap[component.ID, *baseComponent]()
-	assert.Len(t, comps.components, 0)
+	assert.Empty(t, comps.components)
 	myErr := errors.New("my error")
 	_, err := comps.LoadOrStore(
 		id,
 		func() (*baseComponent, error) { return nil, myErr },
 		newNopTelemetrySettings(),
 	)
 	assert.ErrorIs(t, err, myErr)
-	assert.Len(t, comps.components, 0)
+	assert.Empty(t, comps.components)
 }
 
 func TestSharedComponentsLoadOrStore(t *testing.T) {
@@ -65,7 +65,7 @@ func TestSharedComponentsLoadOrStore(t *testing.T) {
 
 	// Shutdown nop will remove
 	assert.NoError(t, got.Shutdown(context.Background()))
-	assert.Len(t, comps.components, 0)
+	assert.Empty(t, comps.components)
 	gotThird, err := comps.LoadOrStore(
 		id,
 		func() (*baseComponent, error) { return nop, nil },
@@ -200,7 +200,7 @@ func TestReportStatusOnStartShutdown(t *testing.T) {
 				require.Equal(t, tc.shutdownErr, err)
 			}
 
-			require.Equal(t, tc.expectedNumReporterInstances, len(reportedStatuses))
+			require.Len(t, reportedStatuses, tc.expectedNumReporterInstances)
 
 			for _, actualStatuses := range reportedStatuses {
 				require.Equal(t, tc.expectedStatuses, actualStatuses)

diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go
@@ -149,14 +149,14 @@ func createExclusionsList(exclusionsText string, t testing.TB) []portpair {
 	var exclusions []portpair
 
 	parts := strings.Split(exclusionsText, "--------")
-	require.Equal(t, len(parts), 3)
+	require.Len(t, parts, 3)
 	portsText := strings.Split(parts[2], "*")
 	require.Greater(t, len(portsText), 1) // original text may have a suffix like " - Administered port exclusions."
 	lines := strings.Split(portsText[0], "\n")
 	for _, line := range lines {
 		if strings.TrimSpace(line) != "" {
 			entries := strings.Fields(strings.TrimSpace(line))
-			require.Equal(t, len(entries), 2)
+			require.Len(t, entries, 2)
 			pair := portpair{entries[0], entries[1]}
 			exclusions = append(exclusions, pair)
 		}

diff --git a/internal/testutil/testutil_test.go b/internal/testutil/testutil_test.go
@@ -68,8 +68,8 @@ Start Port    End Port
 * - Administered port exclusions.
 `
 	exclusions := createExclusionsList(exclusionsText, t)
-	require.Equal(t, len(exclusions), 2)
+	require.Len(t, exclusions, 2)
 
 	emptyExclusions := createExclusionsList(emptyExclusionsText, t)
-	require.Equal(t, len(emptyExclusions), 0)
+	require.Empty(t, emptyExclusions)
 }
diff --git a/internal/tools/go.mod b/internal/tools/go.mod
@@ -5,6 +5,7 @@ go 1.22.1
 toolchain go1.22.6
 
 require (
+	github.com/Antonboom/testifylint v1.4.3
 	github.com/a8m/envsubst v1.4.2
 	github.com/client9/misspell v0.3.4
 	github.com/golangci/golangci-lint v1.60.1
@@ -29,7 +30,6 @@ require (
 	github.com/Abirdcfly/dupword v0.0.14 // indirect
 	github.com/Antonboom/errname v0.1.13 // indirect
 	github.com/Antonboom/nilnil v0.1.9 // indirect
-	github.com/Antonboom/testifylint v1.4.3 // indirect
 	github.com/BurntSushi/toml v1.4.1-0.20240526193622-a339e1f7089c // indirect
 	github.com/Crocmagnon/fatcontext v0.4.0 // indirect
 	github.com/Djarvur/go-err113 v0.0.0-20210108212216-aea10b59be24 // indirect

diff --git a/internal/tools/tools.go b/internal/tools/tools.go
@@ -11,6 +11,7 @@ package tools // import "go.opentelemetry.io/collector/internal/tools"
 // This ensures that all systems use the same version of tools in addition to regular dependencies.
 
 import (
+	_ "github.com/Antonboom/testifylint"
 	_ "github.com/a8m/envsubst/cmd/envsubst"
 	_ "github.com/client9/misspell/cmd/misspell"
 	_ "github.com/golangci/golangci-lint/cmd/golangci-lint"

diff --git a/otelcol/buffered_core_test.go b/otelcol/buffered_core_test.go
@@ -72,7 +72,7 @@ func Test_bufferedCore_Write(t *testing.T) {
 		e,
 		fields,
 	}
-	require.Equal(t, 1, len(bc.logs))
+	require.Len(t, bc.logs, 1)
 	require.Equal(t, expected, bc.logs[0])
 }
 

diff --git a/otelcol/collector_core_test.go b/otelcol/collector_core_test.go
@@ -72,7 +72,7 @@ func Test_collectorCore_Write(t *testing.T) {
 		e,
 		fields,
 	}
-	require.Equal(t, 1, len(cc.core.(*bufferedCore).logs))
+	require.Len(t, cc.core.(*bufferedCore).logs, 1)
 	require.Equal(t, expected, cc.core.(*bufferedCore).logs[0])
 }
 

diff --git a/otelcol/otelcoltest/nop_factories_test.go b/otelcol/otelcoltest/nop_factories_test.go
@@ -17,27 +17,27 @@ func TestNopFactories(t *testing.T) {
 	nopFactories, err := NopFactories()
 	require.NoError(t, err)
 
-	require.Equal(t, 1, len(nopFactories.Receivers))
+	require.Len(t, nopFactories.Receivers, 1)
 	nopReceiverFactory, ok := nopFactories.Receivers[nopType]
 	require.True(t, ok)
 	require.Equal(t, nopType, nopReceiverFactory.Type())
 
-	require.Equal(t, 1, len(nopFactories.Processors))
+	require.Len(t, nopFactories.Processors, 1)
 	nopProcessorFactory, ok := nopFactories.Processors[nopType]
 	require.True(t, ok)
 	require.Equal(t, nopType, nopProcessorFactory.Type())
 
-	require.Equal(t, 1, len(nopFactories.Exporters))
+	require.Len(t, nopFactories.Exporters, 1)
 	nopExporterFactory, ok := nopFactories.Exporters[nopType]
 	require.True(t, ok)
 	require.Equal(t, nopType, nopExporterFactory.Type())
 
-	require.Equal(t, 1, len(nopFactories.Extensions))
+	require.Len(t, nopFactories.Extensions, 1)
 	nopExtensionFactory, ok := nopFactories.Extensions[nopType]
 	require.True(t, ok)
 	require.Equal(t, nopType, nopExtensionFactory.Type())
 
-	require.Equal(t, 1, len(nopFactories.Connectors))
+	require.Len(t, nopFactories.Connectors, 1)
 	nopConnectorFactory, ok := nopFactories.Connectors[nopType]
 	require.True(t, ok)
 	require.Equal(t, nopType, nopConnectorFactory.Type())

diff --git a/pdata/pcommon/map_test.go b/pdata/pcommon/map_test.go
@@ -320,7 +320,7 @@ func TestMap_Range(t *testing.T) {
 		delete(rawMap, k)
 		return true
 	})
-	assert.EqualValues(t, 0, len(rawMap))
+	assert.Empty(t, rawMap)
 }
 
 func TestMap_FromRaw(t *testing.T) {

diff --git a/pdata/pcommon/slice_test.go b/pdata/pcommon/slice_test.go
@@ -77,7 +77,7 @@ func TestSlice_EnsureCapacity(t *testing.T) {
 	for i := 0; i < es.Len(); i++ {
 		expectedEs[es.At(i).getOrig()] = true
 	}
-	assert.Equal(t, es.Len(), len(expectedEs))
+	assert.Len(t, expectedEs, es.Len())
 	es.EnsureCapacity(ensureSmallLen)
 	assert.Less(t, ensureSmallLen, es.Len())
 	foundEs := make(map[*otlpcommon.AnyValue]bool, es.Len())
@@ -89,7 +89,7 @@ func TestSlice_EnsureCapacity(t *testing.T) {
 	// Test ensure larger capacity
 	const ensureLargeLen = 9
 	oldLen := es.Len()
-	assert.Equal(t, oldLen, len(expectedEs))
+	assert.Len(t, expectedEs, oldLen)
 	es.EnsureCapacity(ensureLargeLen)
 	assert.Equal(t, ensureLargeLen, cap(*es.getOrig()))
 }