Enable testifylint linter and fix various issues#3928
Enable testifylint linter and fix various issues#3928kcp-ci-bot merged 2 commits intokcp-dev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables the testifylint golangci-lint linter and updates existing tests to comply with testify best practices (argument ordering, Len/Empty/NoError helpers, etc.), reducing noisy/fragile assertions across the test suite.
Changes:
- Enable
testifylintin.golangci.yaml. - Refactor many tests to use idiomatic testify assertions (
require.NoError,require.Len,require.Empty, correct expected/actual ordering). - Add a targeted
//nolint:testifylintin one place whereassertis intentionally preferred.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .golangci.yaml | Enables testifylint linter. |
| test/integration/framework/server_test.go | Replace require.Nil(err) with require.NoError. |
| test/e2e/workspacetype/controller_test.go | Fix expected/actual order in equality assertions. |
| test/e2e/workspace/inactive_test.go | Remove redundant assert usage and import. |
| test/e2e/workspace/deletion_test.go | Use require.NotNil for deletion timestamps. |
| test/e2e/watchcache/watchcache_enabled_test.go | Prefer require.Len over len(...) comparisons. |
| test/e2e/virtualresources/cachedresources/vr_cachedresources_test.go | Prefer Len and stricter Equal for unstructured comparisons. |
| test/e2e/virtual/terminatingworkspaces/virtualworkspace_test.go | Fix expected/actual ordering; use Len. |
| test/e2e/virtual/initializingworkspaces/virtualworkspace_test.go | Fix expected/actual ordering in watch event type checks. |
| test/e2e/virtual/apiexport/virtualworkspace_test.go | Prefer Len/Empty over manual length checks. |
| test/e2e/virtual/apiexport/binding_test.go | Prefer Empty over Zero(len(...)). |
| test/e2e/server/url_test.go | Keep assert.NoError with a nolint:testifylint rationale. |
| test/e2e/reconciler/workspacedeletion/controller_test.go | Prefer Empty over len(...) == 0. |
| test/e2e/reconciler/partitionset/partitionset_test.go | Prefer Empty and Equal for string comparison. |
| test/e2e/reconciler/cache/helpers.go | Switch to assert.NoError in goroutine and add assert import. |
| test/e2e/reconciler/apiexportendpointslice/apiexportendpointslice_test.go | Prefer Empty over len(...) == 0. |
| test/e2e/authorizer/serviceaccounts_test.go | Remove unnecessary fmt.Sprintf in require.Error messages. |
| test/e2e/apibinding/maximalpermissionpolicy_authorizer_test.go | Prefer Len/Empty over manual length checks. |
| test/e2e/apibinding/apibinding_test.go | Prefer Empty/Len over manual length checks. |
| test/e2e/apibinding/apibinding_deletion_test.go | Replace Equal(true, ...) with True(...). |
| pkg/virtual/apiexport/schemas/builtin/builtin_test.go | Prefer require.Len. |
| pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller_test.go | Fix expected/actual order in require.Equal. |
| pkg/virtual/apiexport/authorizer/maximal_permission_policy_test.go | Fix expected/actual ordering for error string comparison. |
| pkg/virtual/apiexport/authorizer/binding_test.go | Fix expected/actual ordering for error string comparison. |
| pkg/server/apiextensions_test.go | Fix expected/actual ordering in require.Equal. |
| pkg/reconciler/topology/partitionset/partitioning_test.go | Prefer Empty/Len over manual length checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require.Equal(t, 1, len(list.Items), "Unexpected number of items in %s list in %q when listing through %q", resourceName, consumerPath, host) | ||
| require.Len(t, list.Items, 1, "Unexpected number of items in %s list in %q when listing through %q", resourceName, consumerPath, host) | ||
|
|
||
| require.NoError(t, err) |
There was a problem hiding this comment.
There are two consecutive require.NoError(t, err) checks for the same err value. The second one is redundant and can be removed to reduce noise and avoid implying err changed between the calls.
| require.NoError(t, err) |
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: f20264b9595a031e1f19fa9315cdda8c0e933ba5 |
|
/retest === FAIL: test/e2e/reconciler/workspacedeletion TestWorkspaceDeletion/create_and_clean_workspace (30.90s) has been flaking recently =/ |
|
/retest infra failure |
|
/test pull-kcp-test-e2e-sharded === FAIL: test/e2e/workspacetype TestWorkspaceTypes/create_a_workspace_without_an_explicit_type,_get_default_type (33.39s) Also a flake, seen that a lot recently as well. |
|
/retest Mhm, also looks like infra failure. |
|
The user might not have the RBAC yet. |
|
/retest There's already an pr open to fix that |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
golangci-lint has added support for testifylint a few versions back.
It is very very handy and can autofix most things.
What Type of PR Is This?
/kind feature
/kind cleanup
Related Issue(s)
Fixes #
Release Notes