gomega annotation formatting

[pohly]

gomega annotations support printf-formatting and thus should not be passed the result of fmt.Sprintf

The linter could complain about:

  Expect(x).To(Equal(10), fmt.Sprintf("hello world, some other variable %d", y))

That should be

  Expect(x).To(Equal(10), "hello world, some other variable %d", y)

Bonus points if it supports code suggestions and thus automatic fixing of existing code :smile:

[pohly]

I've seen people do this wrong in Kubernetes (https://github.com/kubernetes/kubernetes/issues/115234).

[nunnatsa]

Thanks for raising it @pohly.

I wonder what is the benefit for enforcing this new rule.

The assertion output will be the same and I'm not sure the readability is much better than using the fmt.Sprintf function. my concern is that each new linter rule will make it harder to adopt the linter and to upgrade it. I can add a flag to suppress it, of course, but I'm afraid it will be off in most cases.

WDYT?

[pohly]

My thinking was that To(..., fmt.Sprintf("hello %s", y)) could lead to problems when y contains % and Sprintf gets called again ("missing argument").

But I am not sure anymore whether Gomega will call Sprintf when it's only passed a single string. Knowing @onsi, he probably has considered this. I just don't see it documented:

• https://pkg.go.dev/github.com/onsi/gomega@v1.27.1/types#Assertion
• https://onsi.github.io/gomega/#annotating-assertions

Even if Gomega passes through a single string, I prefer to have it done consistently, and the documentation for Gomega describes it without fmt.Sprintf.

[nunnatsa]

Found it: https://github.com/onsi/gomega/blob/master/internal/assertion.go#L77-L87

func (assertion *Assertion) buildDescription(optionalDescription ...interface{}) string {
    switch len(optionalDescription) {
    case 0:
        return ""
    case 1:
        if describe, ok := optionalDescription[0].(func() string); ok {
            return describe() + "\n"
        }
    }
    return fmt.Sprintf(optionalDescription[0].(string), optionalDescription[1:]...) + "\n"
}

[pohly]

As using fmt.Sprintf is not causing any problems, it becomes a matter of taste. I agree that it should be turned off.

Does the linter support severity levels? I've not investigated how those work, but perhaps they can be used to distinguish between issues that must be fixed and those that only get called out without causing linting to fail?

[pohly]

The golangci-lint severity rules can be used to adjust the level depending on the desired effect.

[pohly]

There's actually one advantage of using fmt.Sprintf: then the linter (go vet?) which checks format string against parameters can do its work.

But I see that as a workaround for a deficiency in linting: ideally, the same check should also be applied when using Gomega's formatting (To(Equal(10), "hello world, some other variable %d", "some string")). Something that ginkgolinter could handle?

[nunnatsa]

Sound interesting. I don't want to copy the govet (?) logic to ginkgolinter. In theory, we can export "fact" about this function call and let govet handle it. But I almost 100% sure that ginkgolinter will run later. Need to check this approach.

[pohly]

And another one (from Kubernetes):

gomega.Expect(s2).To(gomega.BeEmpty(), "global mount points not found: %v", s2)

Including s2 in the format arguments is redundant.

[pohly]

we can export "fact" about this function call and let govet handle it

I was thinking the same, but it could be problematic because the format string itself is hidden behind the interface{} slice, i.e. it's not a function of the usual pattern with func foo(<some args>, format string, args ..interface{}).

[nunnatsa]

Oh. got it - this is why the printf linter can't find it. It looks for functions with 2nd last parameter of type string, and here it's interface{}.

I'm not sure I want to focus on parsing and understanding the assertion description in this linter at this point. I implemented the original idea at #50, but not sure I want to merge it. I want to hear more voices. How often do you find this pattern?

[pohly]

I'm not sure I want to focus on parsing and understanding the assertion description in this linter at this point.

Could the implementation of the printf linter be reused? Just a thought. I understand that this could get complicated.

I implemented the original idea at https://github.com/nunnatsa/ginkgolinter/pull/50, but not sure I want to merge it. I want to hear more voices.

That makes sense. After verifying that passing a single string is okay, I am not convinced myself either. It's subjective.

More important is the printf checking, because that could find real problems that are unambiguously wrong.

How often do you find this pattern?

$ git grep Expect.*Sprintf | wc -l
143

Some of those are false positives, but eyeballing the results confirms that many are legitimate.

[nunnatsa]

Looking at the printf linter code again, it seems that it logic is not exposed and so it's not reusable. This linter also exports facts about function/method declarations, but the gomega assertion methods are not part of the checked code, and can't be marked as printf functions.

I couldn't find a way to reuse this linter in ginkgolinter. If you have an idea how to do that - I'll be glad to hear.

[pohly]

No good idea.

We could ask @onsi to add this to gomega:

func Sprintf(format string, args ...interface{}) func() string {
    if false {
        // A trick to ensure that the printf linter recognizes Sprintf as doing string formatting.
        _ = fmt.Sprint(format, args...)
    }

    return func() string {
        fmt.Sprintf(string, args...)
    }
}

My expectation (to be verified!) is that using this will trigger the format checking, while being more efficient than calling fmt.Sprintf directly.

Then the linter could enforce that the annotation is either:

• a plain string
• a function

For "string + args" it could point towards gomega.Sprintf as replacement.

Still feels like a linter workaround, though :cry: