Refine testify assertions in tests for clearer failures and lint-aligned patterns#4051
Refine testify assertions in tests for clearer failures and lint-aligned patterns#4051
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/b3e143bf-9353-45dd-8d9b-80cebf30c68e Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/b3e143bf-9353-45dd-8d9b-80cebf30c68e Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates Go test code to use more specific testify/assert and require assertions so failures are clearer and more lint-aligned, without changing production behavior.
Changes:
- Replaces low-signal patterns like
assert.Equal(t, true/false, ...)andstrings.Contains(...)boolean checks withassert.True/False,assert.Contains/NotContains, andassert.LessOrEqual. - Replaces a compound
strings.Contains(...) || ...check withassert.Regexp(...)for more direct intent. - Adds a small test-only adapter (
validationErrAsError) to allowrequire.NoErrorstyle checks for*ValidationError.
Show a summary per file
| File | Description |
|---|---|
| test/integration/large_payload_test.go | Uses assert.Contains instead of strings.Contains for clearer failures; drops unused import. |
| internal/server/call_backend_tool_test.go | Uses assert.Contains for error-message substring checks; drops unused import. |
| internal/proxy/rest_backend_caller_tool_test.go | Uses assert.Contains for response text checks; drops unused import. |
| internal/proxy/proxy_test.go | Replaces assert.Equal(false, ...) with assert.False(...) for incomplete_results. |
| internal/middleware/jqschema_integration_test.go | Replaces length-boolean assertions with assert.LessOrEqual for truncation checks. |
| internal/mcp/unmarshal_params_test.go | Uses assert.True for bool field assertion after unmarshal. |
| internal/mcp/tool_result_test.go | Uses assert.True for parsed boolean argument assertion. |
| internal/difc/labels_test.go | Replaces manual strings.Contains checks with assert.Contains/NotContains; drops unused import. |
| internal/config/rules/rules_test.go | Adds validationErrAsError helper and switches some require.Nil sites to require.NoError semantics. |
| internal/config/fetch_and_fix_schema_test.go | Uses assert.True for additionalProperties boolean assertion. |
| internal/config/expand_raw_json_test.go | Uses assert.Contains for error substring checks; drops unused import. |
| internal/config/config_test.go | Replaces a compound substring OR check with assert.Regexp for parse error messaging. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 5
| m := result.(map[string]interface{}) | ||
| assert.Equal(t, float64(2), m["total_count"]) | ||
| assert.Equal(t, false, m["incomplete_results"]) | ||
| assert.False(t, m["incomplete_results"].(bool)) |
There was a problem hiding this comment.
These assertions use direct type assertions (e.g. m["incomplete_results"].(bool)), which will panic if the value is missing or not a bool and will hide useful assertion output. Prefer checking the type first (e.g. v, ok := ...; require.True(t, ok) / require.IsType) and then asserting on v, so test failures stay diagnostic rather than crashing.
| assert.False(t, m["incomplete_results"].(bool)) | |
| incompleteResults, ok := m["incomplete_results"] | |
| require.True(t, ok) | |
| incompleteResultsBool, ok := incompleteResults.(bool) | |
| require.True(t, ok) | |
| assert.False(t, incompleteResultsBool) |
| assert.Equal(t, float64(42), target["int_val"]) | ||
| assert.Equal(t, 3.14, target["float_val"]) | ||
| assert.Equal(t, true, target["bool_val"]) | ||
| assert.True(t, target["bool_val"].(bool)) |
There was a problem hiding this comment.
target["bool_val"].(bool) can panic if unmarshalParams ever returns an unexpected type (which is exactly what this test is validating). Consider asserting the type first (v, ok := ...; require.True(t, ok)) and then assert.True(t, v) so the test fails with a clear message instead of a panic.
| assert.True(t, target["bool_val"].(bool)) | |
| boolVal, ok := target["bool_val"].(bool) | |
| require.True(t, ok, "bool_val should be a bool") | |
| assert.True(t, boolVal) |
| assert.Equal(t, "search term", args["query"]) | ||
| assert.Equal(t, float64(10), args["limit"]) | ||
| assert.Equal(t, true, args["active"]) | ||
| assert.True(t, args["active"].(bool)) |
There was a problem hiding this comment.
This uses args["active"].(bool), which will panic (and skip useful assertion output) if the parsed JSON yields an unexpected type. Prefer v, ok := args["active"].(bool); require.True(t, ok) followed by assert.True(t, v) to keep failures diagnostic.
| assert.True(t, args["active"].(bool)) | |
| active, ok := args["active"].(bool) | |
| require.True(t, ok, "expected active to be a bool") | |
| assert.True(t, active) |
| gpMap := guardPolicies.(map[string]interface{}) | ||
| assert.Equal(t, "object", gpMap["type"], "guard-policies.type should be 'object'") | ||
| assert.Equal(t, true, gpMap["additionalProperties"], "guard-policies.additionalProperties should be true") | ||
| assert.True(t, gpMap["additionalProperties"].(bool), "guard-policies.additionalProperties should be true") |
There was a problem hiding this comment.
gpMap["additionalProperties"].(bool) will panic if the field is absent or not a bool (e.g. if schema generation regresses), which makes the test output less actionable. Consider asserting presence/type first (lookup + require.True(t, ok) / require.IsType) and then assert.True on the typed value.
| assert.True(t, gpMap["additionalProperties"].(bool), "guard-policies.additionalProperties should be true") | |
| additionalProperties, hasAdditionalProperties := gpMap["additionalProperties"] | |
| require.True(t, hasAdditionalProperties, "guard-policies.additionalProperties should be present") | |
| require.IsType(t, true, additionalProperties, "guard-policies.additionalProperties should be a bool") | |
| assert.True(t, additionalProperties.(bool), "guard-policies.additionalProperties should be true") |
| assert.Contains(t, errMsg, "line", "Error should mention line number") | ||
| // Our improved format includes "column" explicitly when ParseError is detected | ||
| assert.True(t, strings.Contains(errMsg, "column") || strings.Contains(errMsg, "line 2"), | ||
| assert.Regexp(t, `column|line 2`, errMsg, |
There was a problem hiding this comment.
The regexp column|line 2 is a bit permissive (e.g. it would also match "line 20" because it contains the substring "line 2"). Consider tightening it with word boundaries or a more specific pattern (e.g. \bcolumn\b|\bline\s+2\b) to avoid false positives.
| assert.Regexp(t, `column|line 2`, errMsg, | |
| assert.Regexp(t, `\bcolumn\b|\bline\s+2\b`, errMsg, |
|
@copilot update the PR based on this review feedback #4051 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/5e78a2b2-2590-435f-9662-ff17be3389fd Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Updated the PR based on that review feedback in commit I addressed the flagged items by adding safe type/presence checks before bool assertions in the affected tests and tightened the regex in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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
assert.Equal(t, true/false, ...)→assert.True(...)/assert.False(...)assert.True(t, strings.Contains(...))→assert.Contains(...)assert.False(t, strings.Contains(...))→assert.NotContains(...)assert.True(t, len(x) <= N)→assert.LessOrEqual(t, len(x), N)assert.Regexp(...)for direct intent.Typed-nil error assertion handling in
rules_test.gorequire.Nil(t, err)sites were updated torequire.NoError(...)semantics while preserving behavior for typed-nil*ValidationErrorreturns.validationErrAsError(err *ValidationError) errorTest import cleanup
stringsimports in files whereassert.Contains/assert.NotContainsreplaced manualstrings.Containscalls.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/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/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)/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 "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/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/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/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)/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", "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: