support wrappers by pohly · Pull Request #217 · nunnatsa/ginkgolinter

pohly · 2025-11-26T17:10:19Z

Description

Previously, linting was only done for function calls that the linter could identify by name as being one of the methods in Gomega that are known to return an Assertion or AsyncAssertion (Expect, Consistently, Eventually, etc.).

Now wrappers are also supported. The basic idea is that any function call which returns any type that implements the Gomega assertion methods (To/Should, etc.) can be treated as the root of a Gomega expression call chain.

Wrappers which take an offset as first parameter are not supported because the linter isn't smart enough to detect that the first parameter is an offset. Such wrappers should not be needed anymore thanks to ginkgo.Helper.

The "missing assertion" error then needs to be a bit more vague because it cannot suggest what the appropriate assertion should be.

Fixes #216

Type of change

[X ] New feature (non-breaking change which adds functionality)
[] This change requires a documentation update

^^^ not sure

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added test case and related test data
Update the functional test

Checklist:

I have performed a self-review of my own code
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I ran make goimports
I ran golangci-lint

@nunnatsa

pohly · 2025-11-26T17:17:44Z

-func (h dotHandler) GetGomegaBasicInfo(expr *ast.CallExpr) (*GomegaBasicInfo, bool) {
-	info := &GomegaBasicInfo{}
+func (h dotHandler) GetGomegaBasicInfo(expr *ast.CallExpr) (info *GomegaBasicInfo) {
+	defer finalizeGetGomegaWrapper(expr, h.pass, h.baseAssertionInterface, h.asyncAssertionInterface, &info)


finalizeGetGomegaWrapper is essentially the common part of dotHandler and nameHandler.

Originally I wanted to keep those because it seemed that they might enable some checks that cannot be supported with generic code. I am not sure anymore, maybe it is possible to drop more code.

If we want to drop the name-based code, then we also need to add support for identifying calls which generate a GomegaMatcher: that is the other usage of GetGomegaBasicInfo.

That idea worked out fine, see separate commit.

But unit testing becomes harder to set up, because the code now really needs a valid analysis.Pass instance. I can do that by setting up a source code directory for analysistest and in a fake analyzer run the unit test code. But before I continue in that direction I would like to hear whether you want to make this change.

I've implemented unit testing again.

pohly · 2025-11-26T17:35:06Z

 		Eventually(slowInt).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(42))                   // valid
 		Eventually(slowInt).WithPolling(time.Second * 2).WithTimeout(time.Millisecond * 100).Should(Equal(42))                   // not valid, if validate-async-intervals flag is set
 		Eventually(slowInt()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(42))                 // want `ginkgo-linter: use a function call in Eventually\. This actually checks nothing, because Eventually receives the function returned value, instead of function itself, and this value is never changed\. Consider using .Eventually\(slowInt\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(Equal\(42\)\). instead`
+		wrapper.MyEventually(slowInt()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(42))       // want `ginkgo-linter: use a function call in MyEventually\. This actually checks nothing, because MyEventually receives the function returned value, instead of function itself, and this value is never changed\. Consider using .wrapper.MyEventually\(slowInt\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(Equal\(42\)\). instead`


I didn't want to replicate all test cases with wrappers, instead I focused on those where wrapping might cause a difference: here it is determining the parameter offset (0 instead of -1).

pohly · 2025-11-26T17:39:33Z

-	origActual := handler.GetActualExpr(origSel)
-	if origActual == nil {
-		return nil, false
+	rootCall := info.RootCall


I did not initially understand that origActual probably means "the original call which is passed the actual value" - that is the reason for the name, isn't it?

I can rename RootCall -> ActualCall, it might be a bit more consistent with the rest of the code base.

We must not modify the ast tree, so we use a clone to perform modifications for the fixes. However, when quering the pass, we mudt use thd original objects from the ast tree. This is why we hold the original object, that we treat as read only, and its clone, side by side. And this is the reason for the naming, to make it clear that we won't modify the origs, and won't use the clones for queries.

So "actual" means "the real instance from the ast"? Then "orig" and "actual" seem a bit redundant - they mean the same thing.

"Actual" means the part of the expression that is the actual value to check, vs. the "Matcher" - the part of the expression that chechs the value. Maybe not the best names, but this is working for me 😄

Okay, so that is what I meant in my initial comment, too. RootCall deviates from that convention, so my proposal to rename it (back) to ActualCall still stands.

I do prefer "actual", mostly because it's used all over, including a package name and so on.

"Actually" (pun intended, sorry), in GomegaBasicInfo it can also be the function which produces a matcher, so it's not always the ActualCall.

I'll keep RootCall in GomegaBasicInfo and check how it's called when retrieved from it, using actualCall as before there.

Fun fact: if the wrapper is called exactly like the method that it is wrapping, then semantic checks for matchers still work (example: wrapper.Equal wrapping gomega.Equal). I think this is useful, even if it is a bit based on guesswork.

coveralls · 2025-11-26T18:22:43Z

Pull Request Test Coverage Report for Build 19815117102

Details

186 of 218 (85.32%) changed or added relevant lines in 10 files are covered.
14 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.4%) to 87.415%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/expression/matcher/matcherwithnest.go	1	2	50.0%
internal/expression/matcher/matcher.go	5	7	71.43%
internal/expression/matcher/multiplematchers.go	4	6	66.67%
internal/expression/expression.go	16	24	66.67%
internal/gomegahandler/handler.go	134	153	87.58%

Files with Coverage Reduction	New Missed Lines	%
internal/expression/expression.go	1	86.34%
internal/expression/matcher/matcher.go	1	91.11%
internal/expression/matcher/multiplematchers.go	1	70.59%
internal/gomegainfo/gomegainfo.go	11	72.5%

Totals
Change from base Build 19623548715:	-0.4%
Covered Lines:	2438
Relevant Lines:	2789

💛 - Coveralls

nunnatsa

Thank @pohly ! you really got into it, and meaningfully improved the code.

Could you please try adding a simple functional test (under tests/)? I didn't try to create directories in the txtar files, but I think it's possible. Do you think you can create a super simple wrapper package and a ginkgo test file to use it?

Also, added a few comments inline.

nunnatsa · 2025-11-27T12:35:22Z

+}
+
+func implements(expr ast.Expr, pass *analysis.Pass, i *types.Interface) bool {
+	if exprType, ok := pass.TypesInfo.Types[expr]; ok && types.Implements(exprType.Type, i) {


Maybe:

Suggested change

if exprType, ok := pass.TypesInfo.Types[expr]; ok && types.Implements(exprType.Type, i) {

exprType, ok := pass.TypesInfo.Types[expr]

return ok && types.Implements(exprType.Type, i)

nunnatsa · 2025-11-27T12:47:56Z

-	origActual := handler.GetActualExpr(origSel)
-	if origActual == nil {
-		return nil, false
+	rootCall := info.RootCall


I do prefer "actual", mostly because it's used all over, including a package name and so on.

nunnatsa · 2025-11-27T12:54:50Z


 import (
 	"go/ast"
+	"go/types"


go/gypes already imported.

nunnatsa · 2025-11-27T12:55:37Z

 	}

-	h := GetGomegaHandler(file, nil)
+	h := GetGomegaHandler(file, &analysis.Pass{Pkg: &types.Package{}})


types.Package{} ==> gotypes,Package{}

nunnatsa · 2025-11-27T16:08:12Z

Another question. Why canceling the Handler interface? The idea is reduce the branching in the main code, and to ask questions we already know the answers for them, because expressions in gomega dot import files, looks different from named import, but in different in the same way for the whole file.

pohly · 2025-11-27T16:19:10Z

Why canceling the Handler interface?

After unifying dotHandler and namedHandler (currently in the last commit) there's only a single implementation. In that case there is no need to wrap it in a Handler interface

On my machine (Arrow Lake, 24 cores) this cuts down test time from ~12s to ~8s.

pohly · 2025-11-28T11:21:14Z

Could you please try adding a simple functional test (under tests/)?

Done. I think I have addressed all comments and cannot think of any other things which might be improved, so perhaps the PR is complete now?

pohly · 2025-11-28T14:10:48Z

I verified that this works in Kubernetes through golangci-lint:

No false positives.
Detects intentional ones:

pkg/controller/devicetainteviction/device_taint_eviction_test.go:2135:2: ginkgo-linter: "Expect": missing assertion method. Expected "To()", "ToNot()" or "NotTo()" (ginkgolinter)
	tCtx.Expect(numWatches.Load())
	^
pkg/controller/devicetainteviction/device_taint_eviction_test.go:2366:4: ginkgo-linter: use Equal with different types: Comparing func() int with int; either change the expected value type if possible, or use the BeEquivalentTo() matcher, instead of Equal() (ginkgolinter)
			tCtx.Assert(controller.workqueue.Len).To(gomega.Equal(0), "work queue empty")
			^
pkg/controller/devicetainteviction/device_taint_eviction_test.go:2367:4: ginkgo-linter: "Assert": missing assertion method. Expected one of "To/NotTo/To" (for Expect assertions) or "Should/ShouldNot" (for Eventually/Consistently assertions) (ginkgolinter)
			tCtx.Assert(listEvents)

nunnatsa

Thanks for fixing the comments.

I Found some more issues - sorry for finding them only now.

Previously, linting was only done for function calls that the linter could identify by name as being one of the methods in Gomega that are known to return an Assertion or AsyncAssertion (Expect, Consistently, Eventually, etc.). Now wrappers are also supported. The basic idea is that any function call which returns any type that implements the Gomega assertion methods (To/Should, etc.) can be treated as the root of a Gomega expression call chain. Wrappers which take an offset as first parameter are not supported because the linter isn't smart enough to detect that the first parameter is an offset. Such wrappers should not be needed anymore thanks to ginkgo.Helper. The "missing assertion" error then needs to be a bit more vague because it cannot suggest what the appropriate assertion should be.

nunnatsa

@pohly - this is a great improvement that I know you wanted for a long time. Thanks for your time and effort!

pohly commented Nov 26, 2025

View reviewed changes

Comment thread testdata/src/a/noassersion/common.go Outdated

pohly commented Nov 26, 2025

View reviewed changes

pohly mentioned this pull request Nov 26, 2025

[Enhancement] detect missing To/Should method calls consistently #216

Closed

pohly force-pushed the assertion-wrappers branch from 42558e1 to f3e67ae Compare November 27, 2025 09:27

nunnatsa reviewed Nov 27, 2025

View reviewed changes

run analyzer tests in parallel

2868205

On my machine (Arrow Lake, 24 cores) this cuts down test time from ~12s to ~8s.

pohly force-pushed the assertion-wrappers branch 3 times, most recently from 412255e to 28e5e2c Compare November 28, 2025 11:20

nunnatsa reviewed Nov 30, 2025

View reviewed changes

Comment thread internal/rules/missingassertionrule.go Outdated

Comment thread internal/rules/missingassertionrule.go Outdated

Comment thread linter/ginkgo_linter.go

Comment thread internal/rules/missingassertionrule.go

Comment thread internal/gomegahandler/handler_test.go

pohly force-pushed the assertion-wrappers branch from 28e5e2c to 85e8d22 Compare December 1, 2025 07:44

nunnatsa approved these changes Dec 1, 2025

View reviewed changes

nunnatsa merged commit bd617ba into nunnatsa:main Dec 1, 2025
2 checks passed

pohly mentioned this pull request Dec 16, 2025

DRA e2e: upgrade/downgrade refactor kubernetes/kubernetes#135664

Merged

	if exprType, ok := pass.TypesInfo.Types[expr]; ok && types.Implements(exprType.Type, i) {
	exprType, ok := pass.TypesInfo.Types[expr]
	return ok && types.Implements(exprType.Type, i)

Conversation

pohly commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nunnatsa Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19815117102

Details

💛 - Coveralls

Uh oh!

nunnatsa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nunnatsa commented Nov 27, 2025

Uh oh!

pohly commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pohly commented Nov 28, 2025

Uh oh!

pohly commented Nov 28, 2025

Uh oh!

nunnatsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nunnatsa left a comment

pohly commented Nov 26, 2025 •

edited

Loading

pohly Nov 27, 2025 •

edited

Loading

nunnatsa Nov 26, 2025 •

edited

Loading

coveralls commented Nov 26, 2025 •

edited

Loading

pohly commented Nov 27, 2025 •

edited

Loading