all: replace reflect.DeepEqual for error comparisons in test files

[thevilledev]

Proposal Details

Description

Currently, several test files across the codebase use reflect.DeepEqual to compare errors. This is not the recommended way to compare errors in Go, as it can be both inefficient and potentially fragile. We should replace these comparisons with proper error comparison methods.

Affected Files

At least the following:

• src/strconv/atof_test.go
• src/strconv/atoi_test.go
• src/errors/join_test.go
• src/fmt/errors_test.go
• src/net/iprawsock_test.go
• src/net/mac_test.go
• src/net/tcpsock_test.go
• src/net/udpsock_test.go
• src/encoding/asn1/asn1_test.go
• src/encoding/base64/base64_test.go
• src/go/build/build_test.go

Proposed Solution

Replace reflect.DeepEqual error comparisons with either:

1. Direct error string comparison using err.Error() for simple cases
2. Custom error comparison functions that handle nil cases and string comparison
3. Use of errors.Is() where appropriate

Implementation Plan

The changes will be submitted as separate PRs grouped by package to make reviews more manageable.

Each PR will:

• Remove usage of reflect.DeepEqual for error comparisons
• Add appropriate error comparison functions where needed
• Maintain existing test coverage and functionality
• Include only the changes related to error comparison

Benefits

• More idiomatic error comparison in tests
• More maintainable and clearer error comparison logic

[ianlancetaylor]

There is no API change here, so this does not have to be a proposal. Taking it out of the proposal process.

[callthingsoff]

It reminds me previous CL https://go-review.googlesource.com/c/go/+/620776

[ianlancetaylor]

Just a note that this should be decided on a case-by-case basis. There is nothing with using reflect.DeepEqual when the concerns about it are understood. And in general we need a good reason to change existing working code. We want to avoid code churn.

[gabyhelp]

Related Issues

• all: remove use of reflect.DeepEqual for testing errors #69948 (closed)
• proposal: errors: make different results from New not reflect.DeepEqual #15910 (closed)
• proposal: x/tools/go/analysis/passes/errorstringcompare: new check that warns when programmers compare against Error() strings #49356 (closed)
• proposal: errors: add Like for testing that error values appear as expected #49172 (closed)

Related Code Changes

• all: remove use of reflect.DeepEqual for testing errors
• strconv: remove use of DeepEqual for testing errors
• net/http: remove use of DeepEqual for testing errors
• reflect: ignore errors.Frame in DeepEqual
• encoding/json: remove use of DeepEqual for testing errors

Related Documentation

• Go Wiki: Go Test Comments > Test Error Semantics

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[dsnet]

As an example, I needed to add back use of reflect.DeepEqual into the "encoding/json" tests in https://golang.org/cl/629517. Prior removal of reflect.DeepEqual unfortunately made the tests weaker in their fidelity. As @ianlancetaylor mentioned, it needs to be decided on a case-by-case basis. In general it is fine to use reflect.DeepEqual on error types from the package itself or an internal package that it depends on.

One could even argue that it's fine to use reflect.DeepEqual on all error types within an entire module. Under that perspective, it's fine for the stdlib to use reflect.DeepEqual anywhere since the stdlib does not depend on any 3rd party modules.

[thevilledev]

Just a note that this should be decided on a case-by-case basis.

Exactly, one of the reasons why I suggested to scope these changes per package. Rationale for each approach (error string comparison, custom comparison, errors.Is(), you name it).

There is nothing with using reflect.DeepEqual when the concerns about it are understood. And in general we need a good reason to change existing working code. We want to avoid code churn.

I think this piece from iprawsock_test.go is a good example. Uses reflect.DeepEqual(err, tt.err) for a full match to the test suite, then (after a nil check) to still confirm that the second call is in fact nil through err != tt.err. You could do a reflect.DeepEqual() on the second call to make it look nice, but in reality it's an overkill for a nil check. The first deep equal check could be a custom comparison through ipAddrEqual() as the more idiomatic option.

I guess considering more targeted or idiomatic checks where appropriate would be the way to go? This way, we avoid unnecessary code churn while still improving clarity and staying true to each package’s real testing needs.