MON-3850: Lint CMO tests #2292
MON-3850: Lint CMO tests #2292
Conversation
openshift-ci
bot
added
the
approved
|
Hmm I can't get |
|
Strange, are you on |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rexagod, simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
How about making use of golangci's govet https://golangci-lint.run/usage/linters/ and enable it for tests? that way we're sure it'll run in CI as well. |
|
New changes are detected. LGTM label has been removed. |
openshift-ci-robot
added
the
jira/valid-reference
|
@rexagod: This pull request references MON-3850 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
.golangci.test.yaml
Outdated
| @@ -0,0 +1,11 @@ | |||
| run: | |||
There was a problem hiding this comment.
How about expressing this is .golangci.yaml itself? I think we can enable linters on tests and then exclude the ones we don't want/want later under issues
There was a problem hiding this comment.
My only concern here is that by switching to issues: from run:, we will potentially lose out on all options that the latter field offers over the former.
There was a problem hiding this comment.
We could switch into run: then, but for now I don't think we'll be needing any of those features. WDYT?
There was a problem hiding this comment.
I've pushed a commit with the proposed changes. However, I'm not sure if there's an "include-only" approach under issues: that would allow us to not duplicate the entire list across run: and issues:. I tried the following with no luck.
issues:
exclude-rules:
# Run some linter only for test files by excluding its issues for everything else.
- path-except: _test\.go
linters:
- govetAlso, not that we use presets, but issues: prevents us from using those as well, just thought it's worth mentioning what we are getting into with this change. I haven't resolved all the lint issues yet, but I'll hop on those once this thread resolves.
There was a problem hiding this comment.
I, personally, have no problem with the disabled linters for tests being enumerated explicitly.
Sure, nothing is set in stone, we can always change the config to adapt to the future needs.
You don't have to fix all the linters errors for tests immediately, you can just start with govet if you want.
Gradually, we can work on reducing that list of "disabled linters for tests", although there might be some that we may never wish to activate for tests...
There was a problem hiding this comment.
I see the golangci-lint job is failing on the govet linter for that test file now: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-monitoring-operator/2292/pull-ci-openshift-cluster-monitoring-operator-master-golangci-lint/1796402383471775744
Can you think of any scenarios that might lead to unnoticed consistency issues? If we add a new linter in the future, we'll include it in the first list and run golang-lint. If it fails on tests, we can either fix the tests if we want to keep it enabled or add it to the second list to disable it. If the linter doesn't fail on tests, that's good and means we're checking the tests as well. If the tests change in the future in a way that breaks the linter, we will always have the option to disable the linter for them.
With versioning (git) and reviews, we'll track every change to that file and ensure that the disabling or enabling of a linter for tests is justified.
And we'll also try to Gradually, we can work on reducing that list of "disabled linters for tests", although there might be some that we may never wish to activate for tests...
There was a problem hiding this comment.
I see the golangci-lint job is failing on the govet linter for that test file now: prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-monitoring-operator/2292/pull-ci-openshift-cluster-monitoring-operator-master-golangci-lint/1796402383471775744
The problem is test/!.*_test.go files not being covered by the configuration.
Can you think of any scenarios that might lead to unnoticed consistency issues? [...] With versioning (git) and reviews, we'll track every change to that file and ensure that the disabling or enabling of a linter for tests is justified.
The committer needs to update two lists now, which are not independent of each other. This is only verified by the manual review process that is subject to human oversight.
If it fails on tests, we can either fix the tests if we want to keep it enabled or add it to the second list to disable it. If the linter doesn't fail on tests, that's good and means we're checking the tests as well. If the tests change in the future in a way that breaks the linter, we will always have the option to disable the linter for them.
Irrespective of linter pass or fail status, we would've added a linter to the tests without knowing about that. Imagine if a couple of linters have no problem with the tests and got in as a result of human oversight, until they do. At that point, we will be alarmed by the CI and either have to drop them or fix all the lint issues. Pragmatically, neither of these should happen, and the code, as well as its toolchain configuration, should always be in a deterministic state at all times.
We brought up the topic of the CI being flaky during the sprint review, and this change will contribute to false alarms that could have been avoided, or better handled. By "better handling", I mean if A added a linter to the first list and not the second one, and the code was merged, while implicitly enabling the linter for tests, and later on, B, while adding some new tests, encountered linter issues because of that linter. B will now have to fix the test lint issues caused by A, or bring this up with the team (if they want to drop the linter for tests, since the linter was implicitly enabled for tests, which may or may not have been the intention in the first place), creating unnecessary iterations and cycles for B.
There was a problem hiding this comment.
I see but I maintain my previous stance on this:
If we add a new linter in the future, we'll include it in the first list and run golang-lint. If it fails on tests, we can either fix the tests if we want to keep it enabled or add it to the second list to disable it. If the linter doesn't fail on tests, that's good and means we're checking the tests as well. If the tests change in the future in a way that breaks the linter, we will always have the option to disable the linter for them.
Additionally, as far as I understand, determinism implies that the linter should yield the same results for the same test code. Of course, if the tests change, we should expect the linter results to change as well. This doesn't only apply to tests, the same principle could apply to the rest of the codebase.
Nothing is irreversible or set in stone, we always have the option to make changes in the future. I'd prefer to keep things simple and as similar as possible to what others do. I've never seen multiple .golangci.yaml files and I don't believe our use case is different from others.
There was a problem hiding this comment.
Let me digress a bit. WRT to the configuration being proposed, how do we enable specific linters that are only intended for tests, and not the codebase?
There was a problem hiding this comment.
Perhaps issues.exclude-rules.path-except is intended for that purpose. Again, I'm not suggesting that the current configuration will meet all our needs in the future. I'm merely proposing that we start with a simple setup and make changes as necessary in the future.
Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
.golangci.yaml
Outdated
| @@ -28,6 +27,7 @@ linters: | |||
| - wastedassign | |||
| - whitespace | |||
| - gci | |||
| - govet | |||
There was a problem hiding this comment.
govet is already enabled in line 17
.golangci.yaml
Outdated
| exclude-rules: | ||
| - path: _test\.go | ||
| linters: | ||
| # Only enable the following linters for test files (commented out): |
There was a problem hiding this comment.
we can remove these comments I think, the list of linters above can be the source of truth.
Makefile
Outdated
| @@ -195,11 +195,11 @@ go-fmt: | |||
|
|
|||
| .PHONY: golangci-lint | |||
| golangci-lint: $(GOLANGCI_LINT_BIN) | |||
| $(GOLANGCI_LINT_BIN) run --verbose --print-resources-usage | |||
| $(GOLANGCI_LINT_BIN) run -c .golangci.yaml --verbose --print-resources-usage | |||
|
I think I pushed pre-maturely here (owing to testing out different configurations). I'll try #2292 (comment) on my local and update the PR. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Posting this here explicitly so GitHub doesn't skip past that: #2292 (comment). |
|
@rexagod: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Adds an explicit
vettarget and also integrates it into theverifyflow to catch any such violations within the source code.