Skip to content

Commit 72cf6d4

Browse files
authored
refactor(tests): Replace assert with require for guard assertions (#46913)
### What does this PR do? Replaces `if !assert.XXX(...) { return }` guard patterns with `require.XXX(...)` across test files. The testify PR stretchr/testify#1481 (merged June 2024, included in testify v1.11.1) made `require` work safely with `*assert.CollectT` inside `EventuallyWithT` callbacks. `CollectT.FailNow()` calls `runtime.Goexit()` which exits the callback goroutine — this is caught by the `EventuallyWithT` retry loop, so the test continues retrying as expected. **Changes across 31 test files (~115 replacements):** - `test/new-e2e/tests/containers/` — `base_test.go`, `ecs_test.go`, `k8s_test.go` - `test/new-e2e/tests/` — `discovery/`, `agent-runtimes/`, `cspm/`, `cws/`, `npm/`, `netpath/`, `remote-config/`, `windows/`, `agent-health/`, `agent-log-pipelines/` - `pkg/network/tracer/`, `pkg/dyninst/`, `pkg/util/`, `comp/core/telemetry/`, etc. - Also removed outdated `// Can be replaced by require... once PR #1481 is merged` comments ### Important: `require` must not be used inside raw goroutines While `require` is safe inside `EventuallyWithT` callbacks (thanks to `CollectT.FailNow()` integration), it must **not** be used inside raw `go func()` blocks or HTTP handler callbacks. [`t.FailNow()`](https://pkg.go.dev/testing#T.FailNow) calls `runtime.Goexit()` to stop the current goroutine. The [official Go `testing` documentation](https://pkg.go.dev/testing#T.FailNow) states: > FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test. `EventuallyWithT` is a special case: testify [runs the condition via `go checkCond()`](https://github.com/stretchr/testify/blob/v1.11.1/assert/assertions.go#L2108), but `CollectT` implements its own `FailNow()` that safely exits the callback goroutine and lets the retry loop continue. **Not converted** (intentionally left as `assert`): - Assertions inside raw `go func()` blocks (e.g., `pkg/util/winutil/eventlog/` tests) - Callbacks/predicates returning `bool` or `error` (not test flow control) - Bodies that only log debug info without returning ### Motivation Using `assert` as a guard is error-prone: if someone forgets the `if !... { return }` wrapper, the test continues and panics when accessing nil/empty results (e.g., `metrics[len(metrics)-1]` on an empty slice). `require` makes the "stop on failure" intent explicit and eliminates the boilerplate. ### Describe how you validated your changes - Verified `require` works correctly inside `EventuallyWithT` callbacks (via `CollectT.FailNow()` → `runtime.Goexit()` caught by retry loop) - Confirmed raw `go func()` call sites were left unchanged - These are mechanical replacements that preserve the exact same test semantics — `require` stops execution on failure, which is exactly what the `if !assert... { return }` pattern was doing manually ### Additional Notes The remaining `if !assert` occurrences across other files were reviewed and intentionally kept because they serve different purposes (returning values from helper functions, setting flags, logging diagnostics without stopping, running inside raw goroutines, etc.). Co-authored-by: nicolas.schweitzer <nicolas.schweitzer@datadoghq.com>
1 parent d1a622c commit 72cf6d4

30 files changed

Lines changed: 119 additions & 335 deletions

File tree

comp/core/telemetry/telemetryimpl/prom_counter_test.go

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/prometheus/client_golang/prometheus"
1212
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
1314
)
1415

1516
func TestPromCounterInitializer(t *testing.T) {
@@ -41,18 +42,12 @@ func TestPromCounterInitializer(t *testing.T) {
4142
counter.InitializeToZero("mycheck", "mystate")
4243

4344
endMetrics, err := promTelemetry.Gather()
44-
if !assert.NoError(t, err) {
45-
return
46-
}
45+
require.NoError(t, err)
4746

48-
if !assert.Equal(t, len(endMetrics), 1) {
49-
return
50-
}
47+
require.Len(t, endMetrics, 1)
5148

5249
metricFamily := endMetrics[0]
53-
if !assert.Equal(t, len(metricFamily.GetMetric()), 1) {
54-
return
55-
}
50+
require.Len(t, metricFamily.GetMetric(), 1)
5651

5752
assert.Equal(t, metricFamily.GetName(), "subsystem_test")
5853

comp/core/telemetry/telemetryimpl/telemetry_test.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,9 @@ func TestCounterInitializer(t *testing.T) {
2424
counter.InitializeToZero("mycheck", "mystate")
2525

2626
startMetrics, err := telemetry.GetRegistry().Gather()
27-
if !assert.NoError(t, err) {
28-
return
29-
}
27+
require.NoError(t, err)
3028

31-
if !assert.Equal(t, len(startMetrics), 1) {
32-
return
33-
}
29+
require.Equal(t, len(startMetrics), 1)
3430

3531
metrics, err := telemetry.GetCountMetric("subsystem", "test")
3632
assert.NoError(t, err)

comp/core/workloadmeta/collectors/internal/process/process_collector_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/golang/mock/gomock"
1919
"github.com/stretchr/testify/assert"
2020
"github.com/stretchr/testify/mock"
21+
"github.com/stretchr/testify/require"
2122
"go.uber.org/fx"
2223

2324
"github.com/DataDog/datadog-agent/comp/core/config"
@@ -692,9 +693,8 @@ func TestProcessDifferentCmdline(t *testing.T) {
692693
// Wait for first collection to complete
693694
assert.EventuallyWithT(t, func(cT *assert.CollectT) {
694695
actualProc, err := c.mockStore.GetProcess(pid)
695-
if !assert.NoError(cT, err) || !assert.NotNil(cT, actualProc) {
696-
return
697-
}
696+
require.NoError(cT, err)
697+
require.NotNil(cT, actualProc)
698698
assert.Equal(cT, []string{"bash"}, actualProc.Cmdline)
699699
}, time.Second, time.Millisecond*100)
700700

@@ -704,9 +704,8 @@ func TestProcessDifferentCmdline(t *testing.T) {
704704
// After exec, the store should have htop, not bash
705705
assert.EventuallyWithT(t, func(cT *assert.CollectT) {
706706
actualProc, err := c.mockStore.GetProcess(pid)
707-
if !assert.NoError(cT, err) || !assert.NotNil(cT, actualProc) {
708-
return
709-
}
707+
require.NoError(cT, err)
708+
require.NotNil(cT, actualProc)
710709
// Critical assertion: cmdline should be updated to htop after exec
711710
assert.Equal(cT, []string{"htop"}, actualProc.Cmdline, "Process cmdline should be updated after exec")
712711
assert.Equal(cT, "htop", actualProc.Name, "Process name should be updated after exec")

pkg/collector/check/stats/stats_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"time"
1313

1414
"github.com/stretchr/testify/assert"
15+
"github.com/stretchr/testify/require"
1516

1617
"github.com/DataDog/datadog-agent/comp/core/telemetry/telemetryimpl"
1718
healthplatformmock "github.com/DataDog/datadog-agent/comp/healthplatform/mock"
@@ -95,9 +96,7 @@ func TestNewStatsStateTelemetryInitialized(t *testing.T) {
9596
NewStats(newMockCheck(), healthplatformmock.Mock(t))
9697

9798
tlmData, err := getTelemetryData()
98-
if !assert.NoError(t, err) {
99-
return
100-
}
99+
require.NoError(t, err)
101100

102101
assert.Contains(
103102
t,

pkg/collector/corechecks/gpu/integrationtests/check_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,8 @@ func assertMetricCase(t *testing.T, metricsByName map[string][]mock.Call, tc met
8282
t.Helper()
8383

8484
calls, ok := metricsByName[tc.name]
85-
if !assert.True(t, ok, "%s metric should be present", tc.name) || !assert.NotEmpty(t, calls, "No calls found for metric %s", tc.name) {
86-
return
87-
}
85+
require.True(t, ok, "%s metric should be present", tc.name)
86+
require.NotEmpty(t, calls, "No calls found for metric %s", tc.name)
8887

8988
for _, call := range calls {
9089
value := call.Arguments[1].(float64)

pkg/compliance/dbconfig/loader_test.go

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/DataDog/datadog-agent/pkg/compliance/types"
2121
"github.com/shirou/gopsutil/v4/process"
2222
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
2324
)
2425

2526
func TestDBConfLoader(t *testing.T) {
@@ -599,9 +600,7 @@ include_dir 'conf.d'
599600
defer stop()
600601

601602
c, ok := LoadPostgreSQLConfig(context.Background(), hostroot, proc)
602-
if !assert.True(t, ok) {
603-
return
604-
}
603+
require.True(t, ok)
605604

606605
configData := c.ConfigData.(map[string]string)
607606
assert.Equal(t, "100", configData["max_connections"])
@@ -665,9 +664,7 @@ work_mem = 16MB
665664

666665
c, ok := LoadPostgreSQLConfig(context.Background(), hostroot, proc)
667666
assert.True(t, ok)
668-
if !assert.NotNil(t, c) {
669-
return
670-
}
667+
require.NotNil(t, c)
671668

672669
configData := c.ConfigData.(map[string]string)
673670
assert.Equal(t, "200", configData["max_connections"])
@@ -698,9 +695,7 @@ include 'circular.conf'
698695

699696
c, ok := LoadPostgreSQLConfig(context.Background(), hostroot, proc)
700697
assert.True(t, ok)
701-
if !assert.NotNil(t, c) {
702-
return
703-
}
698+
require.NotNil(t, c)
704699
// Should not crash, circular protection should kick in
705700
configData := c.ConfigData.(map[string]string)
706701
assert.Equal(t, "100", configData["max_connections"])

pkg/dyninst/end_to_end_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -306,14 +306,12 @@ func runE2ETest(t *testing.T, cfg e2eTestConfig) {
306306
makeTargetStatus(uploader.StatusEmitting, expectedProbeIDs...),
307307
)
308308

309-
assertModuleStats := func(t assert.TestingT, expected actuator.Metrics) {
309+
assertModuleStats := func(t require.TestingT, expected actuator.Metrics) {
310310
stats := ts.module.GetStats()["actuator"].(map[string]any)
311311
exp := expected.AsStats()
312312
gotKeys := slices.Sorted(maps.Keys(stats))
313313
expectedKeys := slices.Sorted(maps.Keys(exp))
314-
if !assert.Equal(t, gotKeys, expectedKeys) {
315-
return
316-
}
314+
require.Equal(t, gotKeys, expectedKeys)
317315
for _, key := range gotKeys {
318316
assert.Equal(t, exp[key], stats[key], "key %s", key)
319317
}

pkg/dyninst/missing_type_recompilation_test.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,9 +177,7 @@ func TestMissingTypeRecompilation(t *testing.T) {
177177
require.EventuallyWithT(t, func(c *assert.CollectT) {
178178
stats := m.GetStats()
179179
actuatorStats, ok := stats["actuator"].(map[string]any)
180-
if !assert.True(c, ok, "actuator stats missing") {
181-
return
182-
}
180+
require.True(c, ok, "actuator stats missing")
183181
assert.Equal(c, uint64(0), actuatorStats["numPrograms"],
184182
"expected numPrograms == 0 after cleanup")
185183
assert.Equal(c, uint64(0), actuatorStats["numProcesses"],

pkg/dyninst/procsubscribe/procscan/proc_stat_reader_test.go

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
"strings"
2020
"testing"
2121

22-
"github.com/stretchr/testify/assert"
2322
"github.com/stretchr/testify/require"
2423
)
2524

@@ -95,28 +94,18 @@ func testCases(t testing.TB) iter.Seq[testCase] {
9594
filename := filepath.Base(entry)
9695
name := strings.TrimSuffix(filename, ".txt")
9796
expected, ok := expectedValues[name]
98-
if !assert.True(t, ok,
99-
"test file %s found but no expected values defined", name,
100-
) {
101-
return
102-
}
97+
require.True(t, ok,
98+
"test file %s found but no expected values defined", name)
10399
delete(expectedValues, name)
104100
data, err := testdataFS.ReadFile(entry)
105-
if !assert.NoError(t, err, "failed to read test data") {
106-
return
107-
}
101+
require.NoError(t, err, "failed to read test data")
108102
procRoot := filepath.Join(tempDir, name, "proc")
109103
pidDir := filepath.Join(procRoot, strconv.FormatInt(int64(expected.pid), 10))
110-
if !assert.NoError(t, os.MkdirAll(pidDir, 0o755)) {
111-
return
112-
}
104+
require.NoError(t, os.MkdirAll(pidDir, 0o755))
113105
statPath := filepath.Join(pidDir, "stat")
114-
if !assert.NoError(
106+
require.NoError(
115107
t,
116-
os.WriteFile(statPath, data, 0o644),
117-
) {
118-
return
119-
}
108+
os.WriteFile(statPath, data, 0o644))
120109
if !yield(testCase{
121110
name: name,
122111
expected: expected,

pkg/fleet/installer/setup/config/write_test.go

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"testing"
1414

1515
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
1617
"golang.org/x/text/encoding"
1718
"golang.org/x/text/encoding/unicode"
1819
)
@@ -362,9 +363,7 @@ api_key: newkey
362363
// The config file may be UTF-16 on Windows
363364
t.Run(tc.name+" (UTF-16)", func(t *testing.T) {
364365
encoded, err := unicode.UTF16(unicode.LittleEndian, unicode.ExpectBOM).NewEncoder().String(tc.initialYAML)
365-
if !assert.NoError(t, err) {
366-
return
367-
}
366+
require.NoError(t, err)
368367
tc.initialYAML = encoded
369368
runWriteConfigTestCase(t, tc)
370369
})
@@ -425,14 +424,10 @@ func TestEnsureUTF8(t *testing.T) {
425424
for _, e := range encodings {
426425
// encode input to new encoding
427426
encoded, err := e.NewEncoder().Bytes(input)
428-
if !assert.NoError(t, err) {
429-
return
430-
}
427+
require.NoError(t, err)
431428
// convert it back to UTF-8
432429
output, err := ensureUTF8(encoded)
433-
if !assert.NoError(t, err) {
434-
return
435-
}
430+
require.NoError(t, err)
436431
assert.Equal(t, input, output)
437432
// assert output does not contain BOM
438433
assert.False(t, bytes.HasPrefix(output, []byte{0xFF, 0xFE}), "output should not have UTF-16LE BOM")

0 commit comments

Comments
 (0)