Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test should fail if extra matchers are given like Should(matcher, extraMatcher1, ...) #560

Closed
eiiches opened this issue Jun 16, 2022 · 3 comments

Comments

@eiiches
Copy link
Contributor

eiiches commented Jun 16, 2022

Currently, the extra matchers given to Should(matcher, extraMatcher1, ...) are silently ignored and the assertion passes successfully (see the example below). This happens when a user forget (...me, actually) to wrap the matchers with SatisfyAll() or other composition matchers. The problem is that this gives the wrong impression to the user that the SUT is working correctly.

It would be great if Gomega can detect this kind of incorrect usage and fail the test, notifying the user that the test should be fixed.

Example: (Gomega 1.19.0)

package main

import (
  . "github.com/onsi/gomega"
  "testing"
)

func TestA(t *testing.T) {
  g := NewWithT(t)
  g.Expect([]int{1}).Should( // missing SatisfyAll
    HaveLen(1),
    ContainElement(2),
  )
}
$ go test .
ok      <import path redacted>    0.003s
@blgm
Copy link
Collaborator

blgm commented Jun 16, 2022

Should(matcher types.GomegaMatcher, optionalDescription ...interface{})

The issue is that the optionalDescription ...interface{} is not validated unless the assertion fails. It seems reasonable that the length and type could always be validated so that usability issues such as the one outlined above could be improved.

@thediveo
Copy link
Collaborator

Can work on a PR; should having a Gomega matcher in the optionalDescription cause a panic or a fail? I would expect the latter, as this is what we do when vetting the actual values. However, optionalDescription is part of the static test, so maybe a panic would be better?

@thediveo
Copy link
Collaborator

This is fixed on Master. Please feel free to reopen if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants