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

dev: group linter implementation and integration tests #4603

Merged
merged 117 commits into from Apr 9, 2024

Conversation

ldez
Copy link
Member

@ldez ldez commented Apr 1, 2024

The goal is to group linter implementation and integration tests to facilitate the maintenance and the creation of linters.
It's the continuation of #3100 and #4578.

Each linter has a dedicated package and a dedicated testdata directory.

It allows running all the tests for one linter easily without needing the make test_linters target.

Also, the package test will now contain only "global" integration tests.

The target make test_linters has been renamed to test_integration.

The configuration structures of linters are still inside the config package because of cyclic imports.
It was not my priority for now because it's already a lot of changes.

The file builder_linter.go is still the center of the linter definition, it's intentional: behind the technical problems (cyclic imports) I think it's a good thing to have a central place to reference and document all the linters.

This approach will not be applied to other pieces (e.g. processors, printers, etc.).

typecheck and nolint/nolintlint are special cases due to their nature (not real linters).

Despite the apparent simplicity of this change, it takes me a lot of time to find a working approach, ensure there is no regression inside the tests, and do files move. (I redid my modifications dozens of times)

One commit, one change.

@ldez ldez added enhancement New feature or improvement topic: cleanup Related to code, process, or doc cleanup labels Apr 1, 2024
@ldez ldez added this to the next milestone Apr 1, 2024
@alexandear
Copy link
Contributor

Is it necessary to rename the make target test_linters to test_integration? This target is frequently used in the documentation and in messages such as #3527 (comment). Could you please share the benefits of using the new make target name?

@ldez
Copy link
Member Author

ldez commented Apr 1, 2024

Is it necessary to rename the make target test_linters to test_integration?

Yes because the target will not run the linter tests.

Could you please share the benefits of using the new make target name?

The "new" make target is the same as before but the linter tests are not run by this target.

Then it's not related to benefits or not, it's just the way it works.

This target is frequently used in the documentation

It's used only one time and it's fixed inside this PR.

test/testshared/install.go Outdated Show resolved Hide resolved
test/testshared/install.go Outdated Show resolved Hide resolved
test/testshared/install.go Outdated Show resolved Hide resolved
test/testshared/install.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
pkg/golinters/gocritic/gocritic.go Outdated Show resolved Hide resolved
@ldez ldez force-pushed the feat/linter-pkg branch 2 times, most recently from 8ce8bf6 to f486fc1 Compare April 2, 2024 12:52
@alexandear alexandear self-requested a review April 2, 2024 14:00
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work, I bet it was tedious so really appreciated! Interesting PR btw, never had this much issues rendering the GitHub diff view 😄

Just a question and dedup suggestion but LGTM! ✅

pkg/golinters/protogetter/testdata/proto/test.go Outdated Show resolved Hide resolved
test/testshared/integration/run.go Show resolved Hide resolved
test/testshared/integration/fix.go Outdated Show resolved Hide resolved
@ldez ldez enabled auto-merge (squash) April 9, 2024 21:26
@ldez ldez merged commit 2c666ed into golangci:master Apr 9, 2024
12 checks passed
@ldez ldez deleted the feat/linter-pkg branch April 9, 2024 21:34
@ldez ldez modified the milestones: next, v1.58 May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement topic: cleanup Related to code, process, or doc cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants