Skip to content

Commit 52f30ba

Browse files
authored
Refine testify assertions in tests for clearer failures and lint-aligned patterns (#4051)
This PR addresses the Go Fan/testify review by replacing low-signal assertion patterns with more specific testify assertions across the reported test areas. The goal is to improve failure diagnostics and align tests with current assertion best practices without changing production behavior. - **Assertion specificity cleanup** - Replaced boolean equality assertions with direct boolean assertions: - `assert.Equal(t, true/false, ...)` → `assert.True(...)` / `assert.False(...)` - Replaced generic substring checks: - `assert.True(t, strings.Contains(...))` → `assert.Contains(...)` - `assert.False(t, strings.Contains(...))` → `assert.NotContains(...)` - Replaced length-comparison boolean assertions: - `assert.True(t, len(x) <= N)` → `assert.LessOrEqual(t, len(x), N)` - Replaced one compound substring OR check with `assert.Regexp(...)` for direct intent. - **Typed-nil error assertion handling in `rules_test.go`** - The five `require.Nil(t, err)` sites were updated to `require.NoError(...)` semantics while preserving behavior for typed-nil `*ValidationError` returns. - Added a tiny adapter helper used only in tests: - `validationErrAsError(err *ValidationError) error` - **Test import cleanup** - Removed now-unused `strings` imports in files where `assert.Contains`/`assert.NotContains` replaced manual `strings.Contains` calls. ```go // Before assert.True(t, strings.Contains(text, "Bug report"), "response text should contain the issue title") // After assert.Contains(t, text, "Bug report", "response text should contain the issue title") ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build850057972/b514/launcher.test /tmp/go-build850057972/b514/launcher.test -test.testlogfile=/tmp/go-build850057972/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s E=3 .cfg /tmp/go-build403-w x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I .cfg TiyY/dyEj6sNqXXBEEx-BTiyY x_amd64/vet --gdwarf-5 g/grpc/internal/-qE -o x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build850057972/b496/config.test /tmp/go-build850057972/b496/config.test -test.testlogfile=/tmp/go-build850057972/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build850057972/b394/vet.cfg @v1.1.3/cpu/x86/-errorsas -I x_amd64/vet --gdwarf-5 .io/otel/baggage-atomic -o x_amd64/vet 2483�� g_.a L_XeGB1fo x_amd64/vet -p vendor/golang.or-atomic -lang=go1.25 x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build772277680/b224/config.test /tmp/go-build772277680/b224/config.test -test.testlogfile=/tmp/go-build772277680/b224/testlog.txt -test.paniconexit0 -test.timeout=10m0s rtcf�� by/280457505e8d1REDACTED y ache/go/1.25.8/x64/pkg/tool/linujson ntime.v2.task/mo/usr/libexec/docker/cli-plugins/docker-buildx -ifaceassert -nilfunc ache/go/1.25.8/x64/pkg/tool/linu--no-legend &#34;SSL�� ef5286be851aaba1bad991c168a5fac9-- -tests ker/cli-plugins/docker-compose by/c68777e185eb8node ache/go/1.25.8/xmock-mcp-server.js x_amd64/vet fce/log.json` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build850057972/b514/launcher.test /tmp/go-build850057972/b514/launcher.test -test.testlogfile=/tmp/go-build850057972/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s E=3 .cfg /tmp/go-build403-w x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I .cfg TiyY/dyEj6sNqXXBEEx-BTiyY x_amd64/vet --gdwarf-5 g/grpc/internal/-qE -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build850057972/b514/launcher.test /tmp/go-build850057972/b514/launcher.test -test.testlogfile=/tmp/go-build850057972/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s E=3 .cfg /tmp/go-build403-w x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I .cfg TiyY/dyEj6sNqXXBEEx-BTiyY x_amd64/vet --gdwarf-5 g/grpc/internal/-qE -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build850057972/b523/mcp.test /tmp/go-build850057972/b523/mcp.test -test.testlogfile=/tmp/go-build850057972/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� 2483721/b343/_pkg_.a -fPIC x_amd64/vet us.pb.go g/grpc/balancer -fmessage-length=0 x_amd64/vet .cfg�� /oidc/errors.go /oidc/provider.go x_amd64/vet -dynout g/grpc/health/gr/usr/bin/runc -ffile-prefix-ma--version x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build772277680/b484/mcp.test /tmp/go-build772277680/b484/mcp.test -test.testlogfile=/tmp/go-build772277680/b484/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool y etc/ssl/certs/ca-certificates.crt&#34;, &#34;REQUESTS_CA_BUNDLE=/etc/ssl/certs/ca-certifi 3cfebab8779a2365/check -ifaceassert -nilfunc noffline 287f�� ef5286be851aaba1bad991c168a5fac9/run/containerd/io.containerd.runtime.v2.task/moby/5878ee59bbb48go 3cfebab8779a2365c0a0927369ddf64be3c02162ae57d53bf27 /usr/bin/grep 9381bd4d1eab940e/usr/bin/runc.original` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents 7ccee20 + bcfd993 commit 52f30ba

12 files changed

Lines changed: 38 additions & 25 deletions

internal/config/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1089,7 +1089,7 @@ port 3000
10891089
errMsg := err.Error()
10901090
assert.Contains(t, errMsg, "line", "Error should mention line number")
10911091
// Our improved format includes "column" explicitly when ParseError is detected
1092-
assert.True(t, strings.Contains(errMsg, "column") || strings.Contains(errMsg, "line 2"),
1092+
assert.Regexp(t, `\bcolumn\b|\bline\s+2\b`, errMsg,
10931093
"Error should mention column or line position, got: %s", errMsg)
10941094
}
10951095

internal/config/expand_raw_json_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package config
22

33
import (
4-
"strings"
54
"testing"
65

76
"github.com/stretchr/testify/assert"
@@ -566,7 +565,7 @@ func TestValidateMounts(t *testing.T) {
566565
if tt.shouldErr {
567566
require.Error(t, err, "Expected an error but got none")
568567
if tt.errMsg != "" {
569-
assert.True(t, strings.Contains(err.Error(), tt.errMsg),
568+
assert.Contains(t, err.Error(), tt.errMsg,
570569
"Error message %q should contain %q", err.Error(), tt.errMsg)
571570
}
572571
} else {

internal/config/fetch_and_fix_schema_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,10 @@ func TestFixSchemaBytes_AddRegistryAndGuardPoliciesToStdioConfig(t *testing.T) {
245245
require.True(t, hasGuardPolicies, "guard-policies field should be added to stdioServerConfig.properties")
246246
gpMap := guardPolicies.(map[string]interface{})
247247
assert.Equal(t, "object", gpMap["type"], "guard-policies.type should be 'object'")
248-
assert.Equal(t, true, gpMap["additionalProperties"], "guard-policies.additionalProperties should be true")
248+
additionalProperties, hasAdditionalProperties := gpMap["additionalProperties"]
249+
require.True(t, hasAdditionalProperties, "guard-policies.additionalProperties should be present")
250+
require.IsType(t, true, additionalProperties, "guard-policies.additionalProperties should be a bool")
251+
assert.True(t, additionalProperties.(bool), "guard-policies.additionalProperties should be true")
249252
}
250253

251254
// TestFixSchemaBytes_AddRegistryAndGuardPoliciesToHttpConfig covers the injection

internal/config/rules/rules_test.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,13 @@ import (
99
"github.com/stretchr/testify/require"
1010
)
1111

12+
func validationErrAsError(err *ValidationError) error {
13+
if err == nil {
14+
return nil
15+
}
16+
return err
17+
}
18+
1219
func TestPortRange(t *testing.T) {
1320
tests := []struct {
1421
name string
@@ -74,7 +81,7 @@ func TestPortRange(t *testing.T) {
7481
assert.Contains(t, err.Message, tt.errMsg, "Error message should contain expected text")
7582
assert.Equal(t, tt.jsonPath, err.JSONPath, "JSONPath should match")
7683
} else {
77-
require.Nil(t, err, "Unexpected validation error")
84+
require.NoError(t, validationErrAsError(err), "Unexpected validation error")
7885
}
7986
})
8087
}
@@ -138,7 +145,7 @@ func TestTimeoutPositive(t *testing.T) {
138145
assert.Equal(t, tt.jsonPath, err.JSONPath, "JSONPath should match")
139146
assert.Equal(t, tt.fieldName, err.Field, "Field name should match")
140147
} else {
141-
require.Nil(t, err, "Unexpected validation error")
148+
require.NoError(t, validationErrAsError(err), "Unexpected validation error")
142149
}
143150
})
144151
}
@@ -295,7 +302,7 @@ func TestMountFormat(t *testing.T) {
295302
assert.Contains(t, err.Message, tt.errMsg, "Error message should contain expected text")
296303
assert.Equal(t, fmt.Sprintf("%s.mounts[%d]", tt.jsonPath, tt.index), err.JSONPath, "JSONPath should match expected pattern")
297304
} else {
298-
require.Nil(t, err, "Unexpected validation error")
305+
require.NoError(t, validationErrAsError(err), "Unexpected validation error")
299306
}
300307
})
301308
}
@@ -899,7 +906,7 @@ func TestNonEmptyString(t *testing.T) {
899906
assert.Contains(t, err.Error(), tt.errMsg)
900907
assert.Contains(t, err.Error(), tt.jsonPath)
901908
} else {
902-
require.Nil(t, err, "Unexpected validation error")
909+
require.NoError(t, validationErrAsError(err), "Unexpected validation error")
903910
}
904911
})
905912
}
@@ -1039,7 +1046,7 @@ func TestAbsolutePath(t *testing.T) {
10391046
assert.Contains(t, err.Error(), tt.errMsg)
10401047
assert.Contains(t, err.Error(), tt.jsonPath)
10411048
} else {
1042-
require.Nil(t, err, "Unexpected validation error")
1049+
require.NoError(t, validationErrAsError(err), "Unexpected validation error")
10431050
}
10441051
})
10451052
}

internal/difc/labels_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package difc
22

33
import (
4-
"strings"
54
"testing"
65

76
"github.com/stretchr/testify/assert"
@@ -590,11 +589,11 @@ func TestViolationError_Error(t *testing.T) {
590589
msg := tt.err.Error()
591590
assert.NotEmpty(t, msg)
592591
for _, want := range tt.wantContains {
593-
assert.True(t, strings.Contains(msg, want),
592+
assert.Contains(t, msg, want,
594593
"expected %q in error message %q", want, msg)
595594
}
596595
for _, absent := range tt.wantAbsent {
597-
assert.False(t, strings.Contains(msg, absent),
596+
assert.NotContains(t, msg, absent,
598597
"expected %q NOT to be in error message %q", absent, msg)
599598
}
600599
})
@@ -616,7 +615,7 @@ func TestViolationError_Detailed(t *testing.T) {
616615
base := err.Error()
617616

618617
// Detailed should contain the base error message
619-
assert.True(t, strings.Contains(detailed, base), "detailed should include base error")
618+
assert.Contains(t, detailed, base, "detailed should include base error")
620619

621620
// Detailed should include agent and resource tag context
622621
assert.Contains(t, detailed, "Agent")

internal/mcp/tool_result_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,9 @@ func TestParseToolArguments(t *testing.T) {
268268
require.NotNil(t, args)
269269
assert.Equal(t, "search term", args["query"])
270270
assert.Equal(t, float64(10), args["limit"])
271-
assert.Equal(t, true, args["active"])
271+
active, ok := args["active"].(bool)
272+
require.True(t, ok, "expected active to be a bool")
273+
assert.True(t, active)
272274
})
273275

274276
t.Run("nested object arguments are parsed correctly", func(t *testing.T) {

internal/mcp/unmarshal_params_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,9 @@ func TestUnmarshalParams(t *testing.T) {
153153
// JSON unmarshal converts numbers to float64 by default
154154
assert.Equal(t, float64(42), target["int_val"])
155155
assert.Equal(t, 3.14, target["float_val"])
156-
assert.Equal(t, true, target["bool_val"])
156+
boolVal, ok := target["bool_val"].(bool)
157+
require.True(t, ok, "bool_val should be a bool")
158+
assert.True(t, boolVal)
157159
assert.Len(t, target["array_val"], 3)
158160
assert.NotNil(t, target["object_val"])
159161
assert.Nil(t, target["null_val"])

internal/middleware/jqschema_integration_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func TestMiddlewareWithLargePayload(t *testing.T) {
195195
// Verify preview truncation in Content field (preview ends with ... when truncated)
196196
previewInContent := contentMap["payloadPreview"].(string)
197197
if strings.HasSuffix(previewInContent, "...") {
198-
assert.True(t, len(previewInContent) <= 503, "Preview in Content should be truncated")
198+
assert.LessOrEqual(t, len(previewInContent), 503, "Preview in Content should be truncated")
199199
}
200200

201201
// Also check data return value
@@ -204,7 +204,7 @@ func TestMiddlewareWithLargePayload(t *testing.T) {
204204
// Verify preview truncation (check if it ends with ...)
205205
preview := dataMap["payloadPreview"].(string)
206206
if strings.HasSuffix(preview, "...") {
207-
assert.True(t, len(preview) <= 503, "Preview should be truncated")
207+
assert.LessOrEqual(t, len(preview), 503, "Preview should be truncated")
208208
}
209209

210210
// Verify payload file has complete data

internal/proxy/proxy_test.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -682,7 +682,11 @@ func TestRewrapSearchResponse(t *testing.T) {
682682
result := rewrapSearchResponse(original, filtered)
683683
m := result.(map[string]interface{})
684684
assert.Equal(t, float64(2), m["total_count"])
685-
assert.Equal(t, false, m["incomplete_results"])
685+
incompleteResults, ok := m["incomplete_results"]
686+
require.True(t, ok)
687+
incompleteResultsBool, ok := incompleteResults.(bool)
688+
require.True(t, ok)
689+
assert.False(t, incompleteResultsBool)
686690
assert.Len(t, m["items"].([]interface{}), 2)
687691
})
688692

internal/proxy/rest_backend_caller_tool_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"fmt"
77
"net/http"
88
"net/http/httptest"
9-
"strings"
109
"testing"
1110

1211
"github.com/stretchr/testify/assert"
@@ -574,5 +573,5 @@ func TestRestBackendCaller_IssueRead_ResponseFormat(t *testing.T) {
574573

575574
text, ok := content[0]["text"].(string)
576575
require.True(t, ok)
577-
assert.True(t, strings.Contains(text, "Bug report"), "response text should contain the issue title")
576+
assert.Contains(t, text, "Bug report", "response text should contain the issue title")
578577
}

0 commit comments

Comments
 (0)