better alternative to BeTrue/False

[pohly]

When using BeTrue or BeFalse, one can annotate the Should or ShouldNot, but then still gets the rather useless Expected <bool>: false to be true. That's noise that makes the failure message harder to read.

New matchers could take a message string and then emit that message string as the failure message, without the Expected: <bool>: false to be true.

The message string could support templating, for those cases where it's unclear whether the matcher is used with Should or ShouldNot.

Tentative names: EqualTrue and EqualFalse.

For details, see https://gophers.slack.com/archives/CQQ50BBNW/p1685525653532099

[thediveo]

I'm wondering if we could simply upgrade in an upwards source-compatible fashion to BeTrue(args ...any) and BeFalse(args ...any)? Since we're talking about templating, should the "Data" element be supported, too?

[pohly]

@thediveo: I prefer separate functions because it enables compile-time checking and simplifies linting.

[onsi]

heyo - so finally getting back to this. i agree with @thediveo in that i’d also want to lean towards BeTrue(args …any) etc. but i appreciate your concern about wanting to enforce a compile-time check/linter rule. At the same time I don’t want to a fuzzy sibling like EqualTrue. How about this for a path forward:

• Be[True|False] can now take an optional (optionalDescription …string) - if provided the description will be emitted instead of the incredibly useless Expected false to be true
• Be[True|False]WithDescription(description string) is a new variant that explicitly requires the description

WDYT?

A few questions for y’all’s thoughts:

It could be Be[True|False](args …any) and the description would then pipe through fmt.Sprintf() if len(args) > 1. Gomega does this in other places so this would be in keeping with a grand tradition.

Negation is tricky here. Obviously we’d prefer the user type Expect(cow.Jumped(moon)).To(BeFalse(“the cow should not jump over the moon”)) over Expect(cow.Jumped(moon)).NotTo(BeTrue(“the cow should jump over the moon”)) - but the latter is technically possible (and sometimes when the user chains matchers together or wraps up a custom defined matcher this sort of thing is possible). What should we do for the negated error message? I propose:

• If the user is making a positive assertion that failed (e.g. To(Be[True|False](…))) then the matcher should just emit the message and only the message.
• if the user is making a negative assertion that failed (e.g. NotTo(Be[True|False](…))) then the matcher should emit:

Expected not true but got true
Negation of “{{message}}” failed

it’s an edge case and its ugly and confusing but short of asking ChatGPT to “negate ” i think it’s the best we can do.

Last thought, Be[True|False]WithDescription works but I can think of other variants that might read well too:

BeFalseBecause(“the cow should not jump over the moon”)
BeTrueBecause(“Kubelet plugins should be registered”)

I’m kind of leaning towards that, actually.

[pohly]

@nunnatsa: if we go the route of extending Be[True|False], would it be okay to extend ginkgolinter so that it optionally (and by default off) warns about plain calls without description?

@onsi: accepting format string and args for it is an invitation to write broken code where the format string doesn't match the parameters. go vet won't be able to detect such errors because it doesn't know that gomega.Be[True|False] does printf-style formatting. I know that this is done elsewhere, but without linter checks I prefer to accept just a simple string and let the user call fmt.Sprintf.

Regarding negation: negation with NotTo makes the check hard to read. I would always replace NotTo(BeTrue()) with To(BeFalse()). Therefore I am not too worried about how it is handled and your proposal seems fine.

[onsi]

sounds good. i’m planning to add the Because variants as well. should be able to get it done soon

[nunnatsa]

@pohly - sound not to hard to implement. My concern is that this is not backward compatible. So yes, the new rule will be off by default, but it's a bit risky.

[pohly]

How about keeping Be[True|False] as-is (no description) and adding func BeTrueF(format string, args....)?

This is not a pattern used in Gomega, but not unusual elsewhere.

The advantage is that the implementation can be marked as a printf-style function using this pattern:

func BeTrueF(format string, args....) ... {
    if false {
        _ = fmt.Sprintf(format, args...) // enable printf checker
    }
    ...
}

Those variants then would have the behavior outlined by @onsi.

I see some advantages with that approach:

• printf checking works with go vet
• In Kubernetes, I can forbid plain Be[True|False] via forbidigo - no need to change ginkgolinter

[onsi]

I'm working on this right now and I'm planning on going with:

// BeTrue succeeds if actual is true
//
// If passed an optional reason, the reason will be displayed instead of the default failure message.
func BeTrue(optionalReason ...string) types.GomegaMatcher {
    return &matchers.BeTrueMatcher{OptionalReason: optionalReason}
}

// BeFalse succeeds if actual is false
//
// If passed an optional reason, the reason will be displayed instead of the default failure message.
func BeFalse(optionalReason ...string) types.GomegaMatcher {
    return &matchers.BeFalseMatcher{OptionalReason: optionalReason}
}

// BeTrueBecause succeeds if actual is true and displays the provided reason if it is false
// If additional arguments are provided fmt.Sprintf is used to render the reason
func BeTrueBecause(format string, args ...any) types.GomegaMatcher {
    reason := format
    if len(args) > 0 {
        reason = fmt.Sprintf(format, args...)
    }
    return BeTrue(reason)
}

// BeFalseBecause succeeds if actual is false and displays the provided reason if it is true.
// If additional arguments are provided fmt.Sprintf is used to render the reason
func BeFalseBecause(format string, args ...any) types.GomegaMatcher {
    reason := format
    if len(args) > 0 {
        reason = fmt.Sprintf(format, args...)
    }
    return BeFalse(reason)
}

[onsi]

Actually as I'm writing the tests I'm learnign that it wil be this:

// BeTrueBecause succeeds if actual is true and displays the provided reason if it is false
// fmt.Sprintf is used to render the reason
func BeTrueBecause(format string, args ...any) types.GomegaMatcher {
    return BeTrue(fmt.Sprintf(format, args...))
}

// BeFalseBecause succeeds if actual is false and displays the provided reason if it is true.
// fmt.Sprintf is used to render the reason
func BeFalseBecause(format string, args ...any) types.GomegaMatcher {
    return BeFalse(fmt.Sprintf(format, args...))
}

As the compiler will actually apply checks to the format string (I tried to do BeTrueBecause("x should be 100%") and it didn't like the %!

[onsi]

lol, and now i'm coming back full circle to just keeping BeTrueBecause and BeFalseBecause and leaving BeTrue and BeFalse alone. The wording flow really nicely with ...Because

[onsi]

Alright, the dust has settled. I have a commit up on master now that:

• leaves BeTrue and BeFalse untouched
• add BeTrueBecause and BeFalseBecause - both take format, args... and should be picked up by the compiler/vet/etc.

Docs were updated too of course. PTAL and i'll cut a release if folks are happy with this.

[pohly]

I was about to comment that go vet won't like single-string parameters because it always expects printf-style, but you already found that! :grin:

The commit in master looks good to me.

[onsi]

great! i’ll cut a release soon

[onsi]

1.30.0 is out now!