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

benchmarks loops should be ignored #13

Closed
lightsgoout opened this issue Mar 21, 2024 · 4 comments · Fixed by #14
Closed

benchmarks loops should be ignored #13

lightsgoout opened this issue Mar 21, 2024 · 4 comments · Fixed by #14
Assignees
Labels
bug Something isn't working

Comments

@lightsgoout
Copy link

Describe the bug
Linter suggests to replace common benchmark loop.

internal/foo/engine/storage/memory/search_bench_test.go:54:2: for loop can be changed to use an integer range (Go 1.22+) (intrange)
	for i := 0; i < b.N; i++ {

Loop like for i := 0; i < b.N; i++ is a standard go benchmark construct.

Additionally, I think it's plain wrong to replace it with int range, because in that case b.N will be evaluated only once, while in for loop b.N is evaluated every iteration, and I think it is important as it allows the bench framework to adapt to execution time.

To Reproduce
Write a bench test.

Expected behavior
Linter should ignore such loops.

@ckaznocha
Copy link
Owner

ckaznocha commented Mar 21, 2024

The side-effect with b.N is certainly an issue. Without making larger changes the linter doesn't have type info so for now it will check for this special convention.

I'll merge the naive fix but may rework it later to account for folks that use benchmarks which break this convention.

@pierrre
Copy link

pierrre commented Sep 17, 2024

Hello

Sorry to re-open this issue, but this statement is wrong/incorrect:

Loop like for i := 0; i < b.N; i++ is a standard go benchmark construct.

Additionally, I think it's plain wrong to replace it with int range, because in that case b.N will be evaluated only once, while in for loop b.N is evaluated every iteration, and I think it is important as it allows the bench framework to adapt to execution time.

If you read the official Go documentation https://pkg.go.dev/testing , it is now recommended to use the new syntax:

for range b.N

They don't mention the old syntax anywhere.

Therefore I think this linter should suggest to rewrite benchmark that use the old syntax:

for i := 0; i < b.N; i++ 

@ckaznocha
Copy link
Owner

Thanks @pierrre you are absolutely correct and I have been planning to revert the change to exclude benchmarks. I think the proper solution here is to expose a setting to exclude patterns. Would you mind opening a new issue? If not I'll make a new issue when I have some time.

To clarify the flaw in the initial issue, go benchmarks don't rely on a side-effect to adjust the number of iterations. The benchmark is evaluated multiple times before a measurement is taken and the value of b.N changes before each evaluation but not during.

@pierrre
Copy link

pierrre commented Sep 22, 2024

Created #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants