Force assertion description rule #200

bootuz · 2025-07-08T08:01:11Z

Description

This PR implements a new linting rule that enforces the use of assertion descriptions in Ginkgo/Gomega tests when enabled via the --force-assertion-description flag.

Changes:

Add AssertionDescriptionRule to detect missing assertion descriptions.
Support both sync (Expect) and async (Eventually, Consistently) assertions.
Comprehensive test coverage.
Documentation updates in README.md and doc.go.
Rule is disabled by default, enabled via CLI.

Fixes #199

Type of change

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

How Has This Been Tested?

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

nunnatsa · 2025-07-08T10:16:31Z

analyzer.go

+		AllowHaveLen0:            false,
+		ForceExpectTo:            false,
+		ForceSucceedForFuncs:     false,
+		ForceOptionalDescription: false,


I find force-optional a bit confusing. I mean, if we're forcing it, it's not optional anymore. WDYT about something like "ForceAseertionDescription", or similar??

yep, that's better name, will change it

internal/rules/rule.go

testdata/src/a/optionaldescription/optionaldesc.go

tests/testdata/optionaldescription.txtar

nunnatsa

@bootuz - thank you so much for this PR. It helps a lot. It looks good in general. I added several minor inline comments.

As a general comment, I think we can come with better naming than optional description. I would go with something like assertion description or something like that. This comment is about naming in general, e.g. variable, type and file names.

Another comment is maybe for the future. We had a request, long time ago, from @pohly , about forcing BeTrue/BeFalse ==> BeTrueBecause/BeFalseBecause. This is very similar to this PR. It's a bit more complicated to implement, though.

And last thing, this PR reminds me about checking the case of using fmt.Sprinf in the description, while the assertion description mechanism already supports it; e.g.

Expect(x).To(Equal(y), fmt.Sprintf("%v must be equal to %v", x, y))

Should be

Expect(x).To(Equal(y), "%v must be equal to %v", x, y)

We can add a new rule to force it. Not in this PR, of course.

nunnatsa · 2025-07-08T12:06:11Z

@bootuz , please also update the README.md file with the new rule, as well as the cli documentation (the cli documentation is in doc.go file in the project root).

pohly · 2025-07-08T12:17:51Z

Should be

Expect(x).To(Equal(y), "%v must be equal to %v", x, y)

Does that still apply format checking, as in fmt.Sprintf("%s", x, y) (too many parameters)?

I don't think the normal linter for format strings does it because it cannot detect that the To parameters are going to be passed to Sprintf. But perhaps the ginkgo linter can close that gap?

Without it, I am reluctant to discourage the use of fmt.Sprintf. It has the drawback that formatting happens whether it's needed or not, but it prevents mistakes that result in broken formatting when it is needed.

nunnatsa · 2025-07-08T13:31:17Z

Should be
Expect(x).To(Equal(y), "%v must be equal to %v", x, y)

Does that still apply format checking, as in fmt.Sprintf("%s", x, y) (too many parameters)?

I don't think the normal linter for format strings does it because it cannot detect that the To parameters are going to be passed to Sprintf. But perhaps the ginkgo linter can close that gap?

Without it, I am reluctant to discourage the use of fmt.Sprintf. It has the drawback that formatting happens whether it's needed or not, but it prevents mistakes that result in broken formatting when it is needed.

The formatting linter (not sure about the name) does not recognizes issues here, as you said. But maybe they expose a function(s) we can use. Worth to check it.

nunnatsa · 2025-07-08T15:54:26Z

@bootuz, few more things. Did you go over the checklist in the description? Only one task is marked as done.

Spiking abput the pr description, please edit it to set issue number (#199), in order to link between the issue and the implementation. It will also auto close the issue when the pr will be mereged.

As said above, we still need to add a not-dot-import test file to the testdata.

And finally, the devision to commits seems random. Please either squash them into one commit, or let each commit do specific part of the change. No need to see the working process here, for example, renaming a file that was added in earlier commit within the same pr. Instead, I would expect that the file will be added in one commit, with the final name.

to enforce assertion descriptions This commit implements a new linting rule that enforces the use of assertion descriptions in Ginkgo/Gomega tests when enabled via the --force-assertion-description flag. Changes: - Add AssertionDescriptionRule to detect missing assertion descriptions - Support both sync (Expect) and async (Eventually, Consistently) assertions. - Comprehensive test coverage including dot imports, named imports, and mixed cases. - Documentation updates in README.md and doc.go - Rule is disabled by default, enabled via CLI.

bootuz · 2025-07-08T16:06:41Z

@nunnatsa Thanks for your swift responses. I think I fixed all the comments. If there's anything else, just let me know!
And thanks for your work, existence of this linter saved me so much time. <3

nunnatsa · 2025-07-08T16:16:38Z

Looks great. Thanks again.

I just noticed that the pr description text is from the template - anf includes the instructions for the contributor ...

Please remove it (2 or 3 places) and add a short explanation of what this pr does (maybe copy parts of the doc?)

bootuz · 2025-07-08T21:16:23Z

@nunnatsa one more question.
I'd like to ask you, could you tell what should be done to make this changes available in golangci-lint? And does it require actions from my side?

nunnatsa · 2025-07-09T03:53:31Z

@nunnatsa one more question. I'd like to ask you, could you tell what should be done to make this changes available in golangci-lint? And does it require actions from my side?

3 steps: first, I'll need to release a new version of the linter, then golangci-lint will automatically adopt it (I'll maybe also need to add the new flag there). After this change is merged in golangci-lint repo, we'll have to wait for the next version release.

nunnatsa · 2025-07-10T15:41:04Z

@bootuz - just released v0.20.0

bootuz mentioned this pull request Jul 8, 2025

[Enhancement] New rule: --force-assertion-description #199

Closed

nunnatsa reviewed Jul 8, 2025

View reviewed changes

internal/rules/rule.go Outdated Show resolved Hide resolved

nunnatsa reviewed Jul 8, 2025

View reviewed changes

testdata/src/a/optionaldescription/optionaldesc.go Outdated Show resolved Hide resolved

nunnatsa reviewed Jul 8, 2025

View reviewed changes

tests/testdata/optionaldescription.txtar Outdated Show resolved Hide resolved

nunnatsa reviewed Jul 8, 2025

View reviewed changes

bootuz requested a review from nunnatsa July 8, 2025 15:53

bootuz closed this Jul 8, 2025

bootuz force-pushed the optional_description_rule branch from ef63513 to de1bcdb Compare July 8, 2025 16:05

bootuz reopened this Jul 8, 2025

bootuz changed the title ~~Optional description rule~~ Force assertion description rule Jul 8, 2025

nunnatsa merged commit ec3014a into nunnatsa:main Jul 8, 2025
2 checks passed

Force assertion description rule #200

Force assertion description rule #200

Uh oh!

Conversation

bootuz commented Jul 8, 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!

nunnatsa Jul 8, 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!

bootuz Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nunnatsa left a comment • 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!

nunnatsa commented Jul 8, 2025

Uh oh!

pohly commented Jul 8, 2025

Uh oh!

nunnatsa commented Jul 8, 2025

Uh oh!

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

Uh oh!

Uh oh!

bootuz commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nunnatsa commented Jul 8, 2025

Uh oh!

Uh oh!

bootuz commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nunnatsa commented Jul 9, 2025

Uh oh!

nunnatsa commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bootuz commented Jul 8, 2025 •

edited

Loading

nunnatsa Jul 8, 2025 •

edited

Loading

nunnatsa left a comment •

edited

Loading

nunnatsa commented Jul 8, 2025 •

edited

Loading

bootuz commented Jul 8, 2025 •

edited

Loading

bootuz commented Jul 8, 2025 •

edited

Loading