Skip to content

Commit

Permalink
[chore]: enable len and empty rules from testifylint (#11021)
Browse files Browse the repository at this point in the history
#### 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 <matthieu.morel35@gmail.com>
  • Loading branch information
mmorel-35 authored Sep 9, 2024
1 parent e599957 commit 6925a30
Show file tree
Hide file tree
Showing 37 changed files with 130 additions and 118 deletions.
2 changes: 0 additions & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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$$" ./
Expand Down
9 changes: 9 additions & 0 deletions Makefile.Common
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 ./...

8 changes: 4 additions & 4 deletions cmd/builder/internal/builder/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions connector/connectorprofiles/profiles_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions connector/forwardconnector/forward_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
4 changes: 2 additions & 2 deletions connector/logs_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions connector/metrics_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions connector/traces_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions consumer/consumertest/sink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand All @@ -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())
}

Expand All @@ -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())
}

Expand All @@ -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())
}
2 changes: 1 addition & 1 deletion exporter/exporterhelper/logs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion exporter/exporterhelper/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion exporter/exporterhelper/traces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions exporter/otlpexporter/otlp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion internal/e2e/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 := ""
Expand Down
10 changes: 5 additions & 5 deletions internal/sharedcomponent/sharedcomponent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 },
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions internal/testutil/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/testutil/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion internal/tools/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions internal/tools/tools.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion otelcol/buffered_core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}

Expand Down
2 changes: 1 addition & 1 deletion otelcol/collector_core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}

Expand Down
10 changes: 5 additions & 5 deletions otelcol/otelcoltest/nop_factories_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion pdata/pcommon/map_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions pdata/pcommon/slice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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()))
}
Expand Down
Loading

0 comments on commit 6925a30

Please sign in to comment.