Skip to content

Commit cb90e2b

Browse files
authored
Replace assert.Nil with assert.NoError for error interfaces (#507)
The Go Fan report identified `assert.Nil(t, err)` usage that should use `assert.NoError(t, err)`. However, this replacement only applies to error interface types, not concrete pointer types like `*ValidationError`. ## Changes - `internal/difc/difc_test.go`: Changed `assert.Nil(t, err)` → `assert.NoError(t, err)` for `FormatViolationError()` which returns `error` interface ## Not Changed Functions returning `*ValidationError` (concrete type) in `internal/config/rules/rules_test.go` correctly use `assert.Nil`: - `PortRange()` - `TimeoutPositive()` - `MountFormat()` ## Rationale Go's typed nil behavior causes `assert.NoError` to fail with concrete pointer types. When a function returns `*ValidationError` (not `error`), a nil return creates a typed nil pointer. Converting this to an `error` interface produces a non-nil interface containing a nil pointer, causing `assert.NoError` to incorrectly report an error. ```go // Correct: concrete pointer type err := PortRange(8080, "path") // returns *ValidationError assert.Nil(t, err) // Works: checks pointer is nil // Correct: error interface err := FormatViolationError(...) // returns error assert.NoError(t, err) // Works: checks interface is nil ``` > [!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: > > - `nonexistent.local` > - Triggering command: `/tmp/go-build2303605970/b273/launcher.test /tmp/go-build2303605970/b273/launcher.test -test.testlogfile=/tmp/go-build2303605970/b273/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true tive\|func MountFormat\|func FormatViolationError -I tnet/tools/as --gdwarf-5 --64 -o 3328745/b183/_x002.o -uns�� ache/go/1.25.6/x64/src/net /tmp/go-build2873328745/b104/vet.cfg e` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2303605970/b282/mcp.test /tmp/go-build2303605970/b282/mcp.test -test.testlogfile=/tmp/go-build2303605970/b282/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1521351387/b260/importcfg -pack /home/REDACTED/.cache/go-build/bb/bb79d626526a1c93efbd89a7ec059b8e00045cd021dd8bc541e1c4e29a277e6a-d 64/pkg/tool/linux_amd64/compile` (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/githubnext/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>[go-fan] Go Module Review: stretchr/testify</issue_title> <issue_description># 🐹 Go Fan Report: testify ## Module Overview Testify is Go's most popular testing toolkit with 25,000+ GitHub stars. It provides comprehensive assertion functions, mocking capabilities, and testing suite interfaces. The project uses v1.11.1 (latest release from August 2025, repository updated January 26, 2026). ## Current Usage in gh-aw-mcpg - **Files**: 67 test files across the entire codebase - **Import Count**: Extensive usage throughout all test packages - **Key APIs Used**: - ✅ `assert` package - widely used for assertions - ✅ `require` package - used for critical assertions that stop test execution - ❌ `mock` package - NOT currently used (0 files) - ❌ `suite` package - NOT currently used (0 files) ### Usage Patterns Observed - **Bound Asserters**: Used in ~30% of test files (`assert.New(t)`, `require.New(t)`) - **Direct Assertions**: Mixed usage of `assert.X(t, ...)` and bound asserters - **Error Handling**: Mostly correct with `require.NoError(t, err)` and `assert.NoError(t, err)` - **Specific Assertions**: Good use of semantic methods like `Equal()`, `Contains()`, `NotNil()` ## Research Findings ### Recent Updates - **v1.11.1** (August 2025): Fixed regression with stringer interface in mock argument matching - **v1.11.0**: Major improvements to mock package, enhanced error messages - **Active Maintenance**: Repository was updated January 26, 2026 (today!) ### Best Practices from Maintainers 1. **Use `require` for critical checks** - test can't continue if assertion fails 2. **Use `assert` for non-critical checks** - gather multiple failures in one test run 3. **Prefer bound asserters** for tests with multiple assertions (cleaner syntax) 4. **Use specific assertion methods**: - `assert.NoError(t, err)` instead of `assert.Nil(t, err)` for errors - `assert.Empty(t, slice)` instead of `assert.True(t, len(slice) == 0)` - `assert.Contains(t, str, substr)` instead of `assert.True(t, strings.Contains(...))` 5. **Enable testifylint** (via golangci-lint) to catch common mistakes automatically ### Current Issues & Trends - **YAML package migration** (Issue #1724): go-yaml/yaml is archived, testify considering migration - **v2 Discussion**: Active discussion about v2, but v1 remains stable with no breaking changes - **testifylint recommended**: Official recommendation to use linter to avoid common mistakes ## Improvement Opportunities ### 🏃 Quick Wins #### 1. Replace `assert.Nil(t, err)` with `assert.NoError(t, err)` - 14 instances Better error messages specifically for errors and semantic clarity. **Files affected**: - `internal/config/rules/rules_test.go` (3 instances) - `internal/sys/sys_test.go` (3 instances) - `internal/logger/common_test.go` (2 instances) - `internal/mcp/http_connection_test.go` (1 instance) - `internal/middleware/jqschema_test.go` (1 instance) - `internal/server/call_tool_result_test.go` (1 instance) - `internal/config/config_test.go` (2 instances) - `internal/difc/difc_test.go` (1 instance) **Example fix**: ```go // ❌ Before assert.Nil(t, err, "Unexpected error") // ✅ After assert.NoError(t, err, "Unexpected error") ``` **Impact**: Better error messages when assertions fail, clearer intent #### 2. Increase Bound Asserter Adoption Only ~30% of test files use bound asserters. For files with 3+ assertions, bound asserters reduce repetition: ```go // ❌ Current pattern (verbose) assert.Equal(t, expected1, actual1) assert.Equal(t, expected2, actual2) assert.True(t, condition) // ✅ Cleaner with bound asserter assert := assert.New(t) assert.Equal(expected1, actual1) assert.Equal(expected2, actual2) assert.True(condition) ``` **Impact**: More readable tests, less repetition, especially in table-driven tests ### ✨ Feature Opportunities #### 1. Consider Mock Package for Interface Testing The project currently doesn't use testify's `mock` package despite having interfaces that could benefit: - Mature and well-integrated with testify - Alternative: Could use [mockery](https://vektra.github.io/mockery/) tool for auto-generating mocks - **Benefit**: Better control over interface behavior in tests, easier to verify interactions #### 2. Explore Suite Package for Integration Tests The `suite` package could benefit `test/integration/` tests with setup/teardown: - Better organization for tests requiring shared state - Built-in setup/teardown hooks - **Caveat**: Suite doesn't support parallel tests (known issue #934) ### 📐 Best Practice Alignment #### 1. Enable testifylint in golangci-lint Currently disabled in `.golangci.yml`. AGENTS.md mentions it requires extensive refactoring, but it would: - Automatically catch `assert.Nil(err)` → `assert.NoError(err)` - Suggest better assertion methods - Enforce consistent patterns **Recommendation**: Enable gradually... </details> > **Custom agent used: agentic-workflows** > GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #471 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
2 parents 49bffb9 + 9d9590f commit cb90e2b

1 file changed

Lines changed: 1 addition & 1 deletion

File tree

internal/difc/difc_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ func TestFormatViolationError(t *testing.T) {
153153
resource := NewLabeledResource("test-resource")
154154

155155
err := FormatViolationError(result, agentSecrecy, agentIntegrity, resource)
156-
assert.Nil(t, err, "Expected nil error for allowed access")
156+
assert.NoError(t, err, "Expected nil error for allowed access")
157157
})
158158

159159
t.Run("formats secrecy violation error", func(t *testing.T) {

0 commit comments

Comments
 (0)