From 0a11a57fd527d47f8b130d0e154b5ba8ea06c26f Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@gmail.com>
Date: Thu, 6 Feb 2020 01:21:44 -0500
Subject: [PATCH] assert: Fix NilError, error non-nil type

The previous fix incorrectly reported non-nil types as nil errors.
Instead this commit handles the case by using a different error message.
---
 assert/assert.go      | 17 +++++++----
 assert/assert_test.go | 68 +++++++++++++++++++++++++++++++++----------
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/assert/assert.go b/assert/assert.go
index 83610aa8..e75d1f38 100644
--- a/assert/assert.go
+++ b/assert/assert.go
@@ -119,12 +119,8 @@ func assert(
 		return true
 
 	case error:
-		// Handle nil structs which implement error as a nil error
-		if reflect.ValueOf(check).IsNil() {
-			return true
-		}
-		msg := "error is not nil: "
-		t.Log(format.WithCustomMessage(failureMessage+msg+check.Error(), msgAndArgs...))
+		msg := failureMsgFromError(check)
+		t.Log(format.WithCustomMessage(failureMessage+msg, msgAndArgs...))
 
 	case cmp.Comparison:
 		success = runComparison(t, argSelector, check, msgAndArgs...)
@@ -179,6 +175,15 @@ func logFailureFromBool(t TestingT, msgAndArgs ...interface{}) {
 	t.Log(format.WithCustomMessage(failureMessage+msg, msgAndArgs...))
 }
 
+func failureMsgFromError(err error) string {
+	// Handle errors with non-nil types
+	v := reflect.ValueOf(err)
+	if v.Kind() == reflect.Ptr && v.IsNil() {
+		return fmt.Sprintf("error is not nil: error has type %T", err)
+	}
+	return "error is not nil: " + err.Error()
+}
+
 func boolFailureMessage(expr ast.Expr) (string, error) {
 	if binaryExpr, ok := expr.(*ast.BinaryExpr); ok && binaryExpr.Op == token.NEQ {
 		x, err := source.FormatNode(binaryExpr.X)
diff --git a/assert/assert_test.go b/assert/assert_test.go
index b703f400..c9758cb4 100644
--- a/assert/assert_test.go
+++ b/assert/assert_test.go
@@ -115,32 +115,70 @@ func TestAssertWithComparisonAndExtraMessage(t *testing.T) {
 	expectFailNowed(t, fakeT, "assertion failed: oops, not good: extra stuff true")
 }
 
-type customError struct{}
+type customError struct {
+	field bool
+}
 
 func (e *customError) Error() string {
+	// access a field of the receiver to simulate the behaviour of most
+	// implementations, and test handling of non-nil typed errors.
+	e.field = true
 	return "custom error"
 }
 
-func TestNilErrorSuccess(t *testing.T) {
-	fakeT := &fakeTestingT{}
+func TestNilError(t *testing.T) {
+	t.Run("nil interface", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		var err error
+		NilError(fakeT, err)
+		expectSuccess(t, fakeT)
+	})
 
-	var err error
-	NilError(fakeT, err)
-	expectSuccess(t, fakeT)
+	t.Run("nil literal", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		NilError(fakeT, nil)
+		expectSuccess(t, fakeT)
+	})
 
-	NilError(fakeT, nil)
-	expectSuccess(t, fakeT)
+	t.Run("interface with non-nil type", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		var customErr *customError
+		NilError(fakeT, customErr)
+		expected := "assertion failed: error is not nil: error has type *assert.customError"
+		expectFailNowed(t, fakeT, expected)
+	})
 
-	var customErr *customError
-	NilError(fakeT, customErr)
-	expectSuccess(t, fakeT)
+	t.Run("non-nil error", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		NilError(fakeT, fmt.Errorf("this is the error"))
+		expectFailNowed(t, fakeT, "assertion failed: error is not nil: this is the error")
+	})
+
+	t.Run("non-nil error with struct type", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		err := structError{}
+		NilError(fakeT, err)
+		expectFailNowed(t, fakeT, "assertion failed: error is not nil: this is a struct")
+	})
+
+	t.Run("non-nil error with map type", func(t *testing.T) {
+		fakeT := &fakeTestingT{}
+		var err mapError
+		NilError(fakeT, err)
+		expectFailNowed(t, fakeT, "assertion failed: error is not nil: ")
+	})
 }
 
-func TestNilErrorFailure(t *testing.T) {
-	fakeT := &fakeTestingT{}
+type structError struct{}
+
+func (structError) Error() string {
+	return "this is a struct"
+}
+
+type mapError map[int]string
 
-	NilError(fakeT, fmt.Errorf("this is the error"))
-	expectFailNowed(t, fakeT, "assertion failed: error is not nil: this is the error")
+func (m mapError) Error() string {
+	return m[0]
 }
 
 func TestCheckFailure(t *testing.T) {