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.

Sign up for GitHub

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

Jump to bottom

Verify linter name in integration tests #1595

Merged
merged 5 commits into from Jan 15, 2021
Merged

Verify linter name in integration tests #1595

merged 5 commits into from Jan 15, 2021

Conversation

ashanbrown
Copy link
Contributor

@ashanbrown ashanbrown commented Dec 29, 2020 •
edited

This PR ensures that the expected linter is triggered in the integration tests (after a case where returned the wrong linter name for a linter -- see #1589).

The PR infers the name of the linter if it is the only explicitly listed linter (enabled with -E<linter>). It can also be specified by adding the argument expected_linter: to the test header or by adding the linter name:

ERROR <linter name> "string"

This PR also enforces a strict syntax for error matches in tests by requiring that a string be Go-syntax string. Note that we still don't verify line number, although we could do that in a later test.

This PR also fixes some incorrect matches in tests where we were just matching a double-quoted subsection of an expected name.

ashanbrown
ashanbrown commented Dec 29, 2020
View reviewed changes
pkg/printers/text.go Outdated
@@ -52,7 +53,7 @@ func (p *Text) Print(ctx context.Context, issues []result.Issue) error {
}

func (p Text) printIssue(i *result.Issue) {
text := p.SprintfColored(color.FgRed, "%s", i.Text)
text := p.SprintfColored(color.FgRed, "%s", strings.TrimSpace(i.Text))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is actually because paralleltest returns issues with newlines at the end, causing us to not have the linter name at the end. paralleltest does not have a customer issue reporter step, so I'm not sure it's wrong to make this adjustment generally.

@ldez ldez added the area: tests Continuous integration, tests and other checks label Dec 29, 2020
ashanbrown
ashanbrown commented Dec 29, 2020
View reviewed changes
test/errchk.go
}
replacedOnce = true
n := lineNum
if strings.HasPrefix(m, "LINE+") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as I could tell, nothing was using this logic that appears to have to do with matching line numbers.

ashanbrown
ashanbrown commented Dec 29, 2020
View reviewed changes
test/testdata/staticcheck_in_megacheck.go
@@ -18,3 +18,9 @@ func StaticcheckNolintMegacheckInMegacheck() {
var x int
x = x //nolint:megacheck
}

func Staticcheck2() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this from stylecheck_in_megacheck since it appeared to be misplaced.

ashanbrown
ashanbrown commented Dec 29, 2020
View reviewed changes
test/linters_test.go
@@ -124,7 +124,6 @@ func testOneSource(t *testing.T, sourcePath string) {
"--allow-parallel-runners",
"--disable-all",
"--print-issued-lines=false",
"--print-linter-name=false",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the linter name to verify it

ashanbrown
ashanbrown commented Dec 29, 2020
View reviewed changes
test/testdata/default_exclude.go
@@ -4,13 +4,13 @@
/*Package testdata ...*/
package testdata

// InvalidFuncComment, both golint and stylecheck will complain about this, // ERROR `ST1020: comment on exported function ExportedFunc1 should be of the form "ExportedFunc1 ..."`
// InvalidFuncComment, both golint and stylecheck will complain about this, // ERROR stylecheck `ST1020: comment on exported function ExportedFunc1 should be of the form "ExportedFunc1 ..."`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

explicitly indicating the expected linter here

@ashanbrown ashanbrown changed the title Verify linter name in integration tests WIP: Verify linter name in integration tests Dec 29, 2020
@ashanbrown ashanbrown marked this pull request as ready for review December 30, 2020 01:30
@ashanbrown ashanbrown changed the title WIP: Verify linter name in integration tests Verify linter name in integration tests Dec 30, 2020
ashanbrown
ashanbrown commented Dec 30, 2020
View reviewed changes
test/testdata/godox.go
@@ -3,11 +3,11 @@
package testdata

func todoLeftInCode() {
// TODO implement me // ERROR godox.go:6: Line contains FIXME/TODO: "TODO implement me"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the only thing being matched here was the stuff between double-quotes.

ashanbrown
ashanbrown commented Dec 30, 2020
View reviewed changes
test/testdata/goerr113.go
@@ -4,17 +4,17 @@ package testdata
import "os"

func SimpleEqual(e1, e2 error) bool {
return e1 == e2 // ERROR `err113: do not compare errors directly, use errors.Is() instead: "e1 == e2"`
return e1 == e2 // ERROR `err113: do not compare errors directly, use errors.Is\(\) instead: "e1 == e2"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change makes this a valid regex in a go string

@ashanbrown ashanbrown added the feedback required Requires additional feedback label Dec 30, 2020
@ldez ldez removed the feedback required Requires additional feedback label Jan 6, 2021
@ldez ldez force-pushed the asb/verify-reported-linter-name branch from 32f6694 to 12dce4b Compare January 10, 2021 22:29
@ldez ldez self-requested a review January 10, 2021 22:30
@ldez ldez force-pushed the asb/verify-reported-linter-name branch from 12dce4b to 1f5492b Compare January 11, 2021 02:04
@ldez
Copy link
Member

ldez commented Jan 11, 2021 •
edited

I wanted to merge the PR so I rebased it, but now the CI is failing on a problem that I don't reproduce...

 === CONT  TestSourcesFromTestdataWithIssuesDir/staticcheck.go
    linters_test.go:37: 
        	Error Trace:	linters_test.go:37
        	            				linters_test.go:158
        	            				linters_test.go:57
        	Error:      	Received unexpected error:
        	            	staticcheck.go:26: missing error "SA1019: runtime.CPUProfile is deprecated"
        	Test:       	TestSourcesFromTestdataWithIssuesDir/staticcheck.go

The CI is going to drive me crazy.

ashanbrown and others added 5 commits January 15, 2021 21:28
@ashanbrown @ldez
Verify linter name in integration tests
38931f1 
Enforce a strict syntax for error matches in tests (although we still don't verify line number).
Also fix some incorrect matches in tests
@ashanbrown @ldez
Fix lint failures
16af771
@ashanbrown @ldez
Fix test
84f1f0b
@ashanbrown @ldez
Fix mismatch
432bacf
@ldez
review
4184b87
@ldez ldez force-pushed the asb/verify-reported-linter-name branch from 1f5492b to 4184b87 Compare January 15, 2021 20:28
ldez
ldez approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit ec46f42 into golangci:master Jan 15, 2021
@ashanbrown
Copy link
Contributor Author

Thanks @ldez!

ashanbrown added a commit to ashanbrown/golangci-lint that referenced this pull request Feb 20, 2021
@ashanbrown
Verify linter name in integration tests (golangci#1595)
0b65d5c
@ldez ldez added this to the v1.36 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldez ldez ldez approved these changes

Assignees
No one assigned
Labels
area: tests Continuous integration, tests and other checks
Projects
None yet
Milestone
v1.36
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ashanbrown @ldez