Tests: use reflect.IsNil() to avoid boxed nil issues (#6305)

Add a new `test.AssertNil()` helper to facilitate asserting that a given
unit test result is a non-boxed nil. Update `test.AssertNotNil()` to use
the reflect package's `.IsNil()` method to catch boxed nils.

In Go, variables whose type is constrained to be an interface type (e.g.
a function parameter which takes an interface, or the return value of a
function which returns `error`, itself an interface type) should
actually be thought of as a (T, V) tuple, where T is their underlying
concrete type and V is their underlying value. Thus, there are two ways
for such a variable to be nil-like: it can be truly nil where T=nil and
V is uninitialized, or it can be a "boxed nil" where T is a nillable
type such as a pointer or a slice and V=nil.

Unfortunately, only the former of these is == nil. The latter is the
cause of frequent bugs, programmer frustration, a whole entry in the Go
FAQ, and considerable design effort to remove from Go 2.

Therefore these two test helpers both call `t.Fatal()` when passed a
boxed nil. We want to avoid passing around boxed nils whenever possible,
and having our tests fail whenever we do is a good way to enforce good
nil hygiene.

Fixes #3279

ca/ca_test.go

@@ -359,7 +359,7 @@ func TestIssuePrecertificate(t *testing.T) {
 				test.AssertNotError(t, err, "Certificate failed to parse")
 
 				poisonExtension := findExtension(cert.Extensions, OIDExtensionCTPoison)
-				test.AssertEquals(t, true, poisonExtension != nil)
+				test.AssertNotNil(t, poisonExtension, "Precert doesn't contain poison extension")
 				if poisonExtension != nil {
 					test.AssertEquals(t, poisonExtension.Critical, true)
 					test.AssertDeepEquals(t, poisonExtension.Value, []byte{0x05, 0x00}) // ASN.1 DER NULL

grpc/errors_test.go

@@ -51,8 +51,8 @@ func TestErrorWrapping(t *testing.T) {
 	test.Assert(t, err != nil, fmt.Sprintf("nil error returned, expected: %s", err))
 	test.AssertDeepEquals(t, err, es.err)
 
-	test.AssertEquals(t, wrapError(context.Background(), nil), nil)
-	test.AssertEquals(t, unwrapError(nil, nil), nil)
+	test.AssertNil(t, wrapError(context.Background(), nil), "Wrapping nil should still be nil")
+	test.AssertNil(t, unwrapError(nil, nil), "Unwrapping nil should still be nil")
 }
 
 // TestSubErrorWrapping tests that a boulder error with suberrors can be

privatekey/privatekey_test.go

@@ -57,6 +57,6 @@ func TestLoad(t *testing.T) {
 
 	signer, public, err = Load("../test/hierarchy/ee-e1.cert.pem")
 	test.AssertError(t, err, "Should have failed, file is a certificate")
-	test.AssertEquals(t, signer, nil)
-	test.AssertEquals(t, public, nil)
+	test.AssertNil(t, signer, "Signer should be nil")
+	test.AssertNil(t, public, "Public should be nil")
 }

sa/sa_test.go

@@ -1483,7 +1483,7 @@ func TestGetOrderForNames(t *testing.T) {
 	// It should not error since a ready order can be reused.
 	test.AssertNotError(t, err, "sa.GetOrderForNames returned an unexpected error for ready order reuse")
 	// The order returned should have the same ID as the order we created above
-	test.AssertEquals(t, result != nil, true)
+	test.AssertNotNil(t, result, "sa.GetOrderForNames returned nil result")
 	test.AssertEquals(t, result.Id, order.Id)
 
 	// Set the order processing so it can be finalized

test/asserts.go

@@ -22,12 +22,31 @@ func Assert(t *testing.T, result bool, message string) {
 	}
 }
 
-// AssertNotNil checks an object to be non-nil
+// AssertNil checks that an object is nil. Being a "boxed nil" (a nil value
+// wrapped in a non-nil interface type) is not good enough.
+func AssertNil(t *testing.T, obj interface{}, message string) {
+	t.Helper()
+	if obj != nil {
+		t.Fatal(message)
+	}
+}
+
+// AssertNotNil checks an object to be non-nil. Being a "boxed nil" (a nil value
+// wrapped in a non-nil interface type) is not good enough.
+// Note that there is a gap between AssertNil and AssertNotNil. Both fail when
+// called with a boxed nil. This is intentional: we want to avoid boxed nils.
 func AssertNotNil(t *testing.T, obj interface{}, message string) {
 	t.Helper()
 	if obj == nil {
 		t.Fatal(message)
 	}
+	switch reflect.TypeOf(obj).Kind() {
+	// .IsNil() only works on chan, func, interface, map, pointer, and slice.
+	case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
+		if reflect.ValueOf(obj).IsNil() {
+			t.Fatal(message)
+		}
+	}
 }
 
 // AssertNotError checks that err is nil