vet optional description args in assertions, fixing #560 (#566)

Author: thediveo

internal/assertion.go

@@ -45,26 +45,31 @@ func (assertion *Assertion) Error() types.Assertion {
 
 func (assertion *Assertion) Should(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *Assertion) ShouldNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 
 func (assertion *Assertion) To(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *Assertion) ToNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 
 func (assertion *Assertion) NotTo(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Assertion", optionalDescription...)
 	return assertion.vet(assertion, optionalDescription...) && assertion.match(matcher, false, optionalDescription...)
 }
 

internal/assertion_test.go

@@ -2,6 +2,7 @@ package internal_test
 
 import (
 	"errors"
+	"reflect"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -189,4 +190,33 @@ var _ = Describe("Making Synchronous Assertions", func() {
 		),
 	)
 
+	When("vetting optional description parameters", func() {
+
+		It("panics when Gomega matcher is at the beginning of optional description parameters", func() {
+			ig := NewInstrumentedGomega()
+			for _, expectator := range []string{
+				"To", "NotTo", "ToNot",
+				"Should", "ShouldNot",
+			} {
+				Expect(func() {
+					expect := ig.G.Expect(42) // sic!
+					meth := reflect.ValueOf(expect).MethodByName(expectator)
+					Expect(meth.IsValid()).To(BeTrue())
+					meth.Call([]reflect.Value{
+						reflect.ValueOf(HaveLen(1)),
+						reflect.ValueOf(ContainElement(42)),
+					})
+				}).To(PanicWith(MatchRegexp("Assertion has a GomegaMatcher as the first element of optionalDescription")))
+			}
+		})
+
+		It("accepts Gomega matchers in optional description parameters after the first", func() {
+			Expect(func() {
+				ig := NewInstrumentedGomega()
+				ig.G.Expect(42).To(HaveLen(1), "foo", ContainElement(42))
+			}).NotTo(Panic())
+		})
+
+	})
+
 })

internal/async_assertion.go

@@ -104,11 +104,13 @@ func (assertion *AsyncAssertion) WithPolling(interval time.Duration) types.Async
 
 func (assertion *AsyncAssertion) Should(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Asynchronous assertion", optionalDescription...)
 	return assertion.match(matcher, true, optionalDescription...)
 }
 
 func (assertion *AsyncAssertion) ShouldNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) bool {
 	assertion.g.THelper()
+	vetOptionalDescription("Asynchronous assertion", optionalDescription...)
 	return assertion.match(matcher, false, optionalDescription...)
 }
 

internal/async_assertion_test.go

@@ -2,6 +2,7 @@ package internal_test
 
 import (
 	"errors"
+	"reflect"
 	"runtime"
 	"time"
 
@@ -712,4 +713,33 @@ var _ = Describe("Asynchronous Assertions", func() {
 			Ω(ig.FailureMessage).Should(ContainSubstring("Timed out after"))
 		})
 	})
+
+	When("vetting optional description parameters", func() {
+
+		It("panics when Gomega matcher is at the beginning of optional description parameters", func() {
+			ig := NewInstrumentedGomega()
+			for _, expectator := range []string{
+				"Should", "ShouldNot",
+			} {
+				Expect(func() {
+					eventually := ig.G.Eventually(42) // sic!
+					meth := reflect.ValueOf(eventually).MethodByName(expectator)
+					Expect(meth.IsValid()).To(BeTrue())
+					meth.Call([]reflect.Value{
+						reflect.ValueOf(HaveLen(1)),
+						reflect.ValueOf(ContainElement(42)),
+					})
+				}).To(PanicWith(MatchRegexp("Asynchronous assertion has a GomegaMatcher as the first element of optionalDescription")))
+			}
+		})
+
+		It("accepts Gomega matchers in optional description parameters after the first", func() {
+			Expect(func() {
+				ig := NewInstrumentedGomega()
+				ig.G.Eventually(42).Should(HaveLen(1), "foo", ContainElement(42))
+			}).NotTo(Panic())
+		})
+
+	})
+
 })

internal/vetoptdesc.go

@@ -0,0 +1,22 @@
+package internal
+
+import (
+	"fmt"
+
+	"github.com/onsi/gomega/types"
+)
+
+// vetOptionalDescription vets the optional description args: if it finds any
+// Gomega matcher at the beginning it panics. This allows for rendering Gomega
+// matchers as part of an optional Description, as long as they're not in the
+// first slot.
+func vetOptionalDescription(assertion string, optionalDescription ...interface{}) {
+	if len(optionalDescription) == 0 {
+		return
+	}
+	if _, isGomegaMatcher := optionalDescription[0].(types.GomegaMatcher); isGomegaMatcher {
+		panic(fmt.Sprintf("%s has a GomegaMatcher as the first element of optionalDescription.\n\t"+
+			"Do you mean to use And/Or/SatisfyAll/SatisfyAny to combine multiple matchers?",
+			assertion))
+	}
+}