Linter issues

[imiric]

Closing this issue should be done when all existing issues are fixed and we remove --new-from-rev from the CI job.

TODO:

• Fix general linter errors xk6-browser#211
• Reenable the new-from-rev option: Revert "Remove BASEREV and new-from-rev from CI workflow" xk6-browser#220
• Set a list of linters and fix some: Add golangci-lint configuration xk6-browser#216
• Fix/linter errs test browser xk6-browser#245
• Fix linter errs and Refactor chromium pkg xk6-browser#246
• Properly fix and remove all nolint directives
• cyclop
• deadcode
• dogsled
• dupl
• durationcheck
• errcheck
• errchkjson
• errname
• errorlint
• exhaustive
• exportloopref
• forbidigo
• forcetypeassert: Fix type assertions and remove nolint directives for good xk6-browser#425
• funlen
• gci
• gochecknoglobals
• gocognit
• goconst
• gocritic
• godot
• gofmt
• gofumpt
• goimports
• gosec
• gosimple
• govet
• ifshort
• importas
• ineffassign
• lll
• makezero
• misspell
• nakedret
• nestif
• nilerr
• nilnil
• nlreturn
• nolintlint
• prealloc
• predeclared
• revive
• staticcheck
• structcheck
• tagliatelle
• thelper
• tparallel
• typecheck
• unconvert
• unparam
• unused
• varcheck
• wastedassign
• whitespace
• wrapcheck

[imiric]

Another issue is that wrong or missing comments on exported declarations aren't being checked. See grafana/xk6-browser#65 (comment).

So it seems that go vet isn't setup properly. Though we might want to run something more sophisticated like Staticcheck instead, which also includes this warning.

[inancgumus]

@imiric Is this PR (#211) also closing this issue?

[imiric]

I think we should look into why go vet isn't working properly.

[inancgumus]

So maybe update the issue description and also change its name to "vet isn't working properly" since we fixed the linter errors? Or create another issue as you explained as "another issue" here :)

Another issue is that wrong or missing comments on exported declarations aren't being checked.

[imiric]

I would classify go vet under "linter issues", since we don't run it directly, but via golangci-lint.

Actually, I'm not sure the missing comments check was done by go vet, it seems to be from golint, which is now deprecated in favor of revive(?).

I'm not sure what the status is or why we're not getting those warnings, but we should look into it under this issue. I don't think we need a new one to track this. This comment should be enough to "expand the scope" of what needs to be done here.

[inancgumus]

But we still fixed the following and I think we better change the issue description and link it to that comment maybe:

The lint job in CI currently ignores errors that are already on main, which is a questionable practice carried over from k6. :)
There's not a lot of them so they're fixable, and we should change the job to run without --new-from-rev.

No? No big deal but can be confusing later on (doubt it :)).

[imiric]

So grafana/xk6-browser#216 adds the missing linters, so we now get errors on missing/wrong comments via revive.

Considering that without --new-from-rev there are 1298 issues with that configuration, do we want to close this issue or keep it open?

I vote for keeping it open, as it will be a constant reminder we need to gradually fix all of these. The "Definition of Done", if you will, should be removing --new-from-rev from CI.

[inancgumus]

I vote for keeping it open, as it will be a constant reminder we need to gradually fix all of these.

Same. That's why I added a todo list to this issue :)

Next week, I'm planning to update the TODO list and add the linters as tasks. So we can track each week what we're accomplishing (as we agreed: one day per week schedule). Some of the tasks can be linterA part I, part II, ... style because some of them have a lot of warnings.

The "Definition of Done", if you will, should be removing --new-from-rev from CI.

I totally agree with you.

[inancgumus]

@imiric I updated the TODO list, here. Please mark the ones you already fixed.

[imiric]

I don't think I fixed any of them without --new-from-rev. The fixes in grafana/xk6-browser#216 were all with --new-from-rev.

[imiric]

Something we just noticed in grafana/xk6-browser#252 is that at least tparallel is not being triggered when t.Parallel() is missing. Running golangci-lint run --out-format=tab --new-from-rev=main ./... shows 0 errors after removing t.Parallel() from the subtest. It doesn't trigger when removing it from the main test either... 😕 Adding //nolint:tparallel to the subtest triggers the nolintlint linter, so it seems something is wonky with that specific integration.

Instead of wrestling with this specific issue, we might want to just switch to the GitHub Action and see if that fixes it. See #2397. Though it would still be a problem when running it locally... 😮‍💨