Skip to content

Enable testifylint to lint test files, and mechanically fix reported issues #35237

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
Merged
merged 17 commits into from
Oct 10, 2024
Merged

Enable testifylint to lint test files, and mechanically fix reported issues #35237

merged 17 commits into from
Oct 10, 2024

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Oct 4, 2024 •
edited
Loading

Enable testifylint to lint the usages of github.com/stretchr/testify, in an effort to improve the readability of tests (and failure outputs) and reduce the chance of flakes. This also aims to remove the need for mechanical code review comments to encourage the usage of idiomatic testify primitives.

The current configuration disables the "float-compare", "go-require" and "require-error" checkers, as extra effort is required to fix the issues reported by them (require-error appears to also lead to false positives when used inside EventuallyWithT blocks).

Note to the reviewers, all except the first five commits, and the last one, have been mechanically generated with the following script:

#!/bin/bash

while read -r LINTER; do

[[ ${LINTER} =~ ^# ]] && continue

# Required to prevent error-nil from attempting to apply the same fix twice to
# a single code-block, failing due to conflicts.
if [[ ${LINTER} = "error-nil" ]]; then
    find . -iname '*_test.go' | \
        xargs sed -i -e 's/require.Nil(t, testutils.WaitUntil/require.NoError(t, testutils.WaitUntil/'
fi

testifylint --fix --disable-all --enable ${LINTER} ./...
git status -s | awk '{ print $2}' | xargs -r goimports -w

git add .
git commit --signoff -F- <<EOF
treewide(tests): testifylint "${LINTER}" fixes

Generated by:

    testifylint --fix --disable-all --enable ${LINTER} ./...
EOF

done <<-EOF
error-nil
bool-compare
compares
contains
empty
error-is-as
expected-actual
formatter
len
negative-positive
nil-compare
regexp
suite-broken-parallel
suite-dont-use-pkg
suite-extra-assert-call
suite-thelper
EOF

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 4, 2024
@github-actions github-actions bot added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. cilium-cli This PR contains changes related with cilium-cli labels Oct 4, 2024
@giorio94
Copy link
Member Author

giorio94 commented Oct 4, 2024

/ci-integration

@giorio94 giorio94 added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Oct 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 4, 2024
@giorio94
Copy link
Member Author

giorio94 commented Oct 4, 2024

/ci-integration

@giorio94 giorio94 marked this pull request as ready for review October 4, 2024 17:13
@giorio94 giorio94 requested review from a team as code owners October 4, 2024 17:13
@julianwiedmann julianwiedmann added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 10, 2024
@julianwiedmann
Copy link
Member

Note this needs a rebase.

giorio94 added 17 commits October 10, 2024 08:58
@giorio94
ipam: fix Test_MultiPoolManager test
75fe85c 
In preparation for the subsequent testifylint mechanical changes, let's
convert an incorrect Error assertion into the corresponding ErrorContains
one. Error asserts that the second argument is a non-nil error, which in
this case was always true.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
bandwidth: fix TestOps
0a91ee8 
In preparation for the subsequent testifylint mechanical changes, let's
fix an incorrect NoError assertion, so that it actually checks the
error returned by LinkSetUp, rather than the error already validated
by the previous assertion.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
maglev: minor change in TestWeightedBackendWithRemoval test
5ea6b7f 
In preparation for the subsequent testifylint mechanical changes, let's
anticipate these modifications. The mechanical ones would indeed lead
to a failing test due to using Equal rather than EqualValues, which also
asserts type equality (map values are of type uint64 here).

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
policy: minor change in TestSelectorManagerCanGetBeforeSet test
6eb1ed8 
In preparation for the subsequent testifylint mechanical changes, let's
anticipate these modifications. The mechanical ones would indeed lead
to a failing test when asserting that the `selections` variable is nil,
because the zero type of a slice is not equal to nil, but satisfies
assert.Nil. Hence, let's just check that the slice is correctly empty.

For reference, all assertions in the following test pass:

    func TestFoo(t *testing.T) {
	var foo []int
	assert.NotEqual(t, nil, foo)
	assert.Nil(t, foo)
	assert.Empty(t, foo)
    }

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
api: drop unnecessary assertions from TestKafkaEqual test
61f4141 
Drop unnecessary assertions, as they were basically testing constants.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "error-nil" fixes
a974827 
Generated by:

    testifylint --fix --disable-all --enable error-nil ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "bool-compare" fixes
46041b0 
Generated by:

    testifylint --fix --disable-all --enable bool-compare ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "compares" fixes
0c81029 
Generated by:

    testifylint --fix --disable-all --enable compares ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "contains" fixes
5bdccfc 
Generated by:

    testifylint --fix --disable-all --enable contains ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "empty" fixes
3b66972 
Generated by:

    testifylint --fix --disable-all --enable empty ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "error-is-as" fixes
e1eed0c 
Generated by:

    testifylint --fix --disable-all --enable error-is-as ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "expected-actual" fixes
596b5ef 
Generated by:

    testifylint --fix --disable-all --enable expected-actual ./...

Manually restored the comments in one of the TestParseGetLabelValues
assertions, which got incorrectly deleted by the mechanical fix.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "formatter" fixes
aec3e4c 
Generated by:

    testifylint --fix --disable-all --enable formatter ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "len" fixes
3023c84 
Generated by:

    testifylint --fix --disable-all --enable len ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "negative-positive" fixes
81f4668 
Generated by:

    testifylint --fix --disable-all --enable negative-positive ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
treewide(tests): testifylint "nil-compare" fixes
13c4822 
Generated by:

    testifylint --fix --disable-all --enable nil-compare ./...

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
golangci-lint: enable testifylint
5e82466 
Enable testifylint to lint the usages of github.com/stretchr/testify,
in an effort to improve the readability of tests (and failure outputs)
and reduce the chance of flakes. This also aims to remove the need for
mechanical code review comments to encourage the usage of idiomatic
testify primitives.

The current configuration disables the "float-compare", "go-require"
and "require-error" checkers, as extra effort is required to fix the
issues reported by them (require-error appears to also lead to false
positives when used inside EventuallyWithT blocks).

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
Copy link
Member Author

Note this needs a rebase.

Yeah, this PR turned out being a conflicts magnet (not that I expected anything different). Let's try again.

@giorio94 giorio94 removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 10, 2024
@giorio94
Copy link
Member Author

/test

@aanm aanm disabled auto-merge October 10, 2024 08:01
@aanm aanm merged commit 3c697b1 into cilium:main Oct 10, 2024
62 of 63 checks passed
@giorio94 giorio94 deleted the mio/testifylint branch October 10, 2024 08:15
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 10, 2024
@giorio94 giorio94 mentioned this pull request Oct 10, 2024
Merged
8 tasks
@github-actions github-actions bot mentioned this pull request Nov 11, 2024
Open
@giorio94 giorio94 mentioned this pull request Mar 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@rolinh rolinh rolinh approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@marseel marseel marseel approved these changes

@christarazi christarazi christarazi approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@jschwinger233 jschwinger233 jschwinger233 approved these changes

@sayboras sayboras sayboras approved these changes

@rastislavs rastislavs rastislavs approved these changes

@rgo3 rgo3 rgo3 approved these changes

Assignees
No one assigned
Labels
cilium-cli This PR contains changes related with cilium-cli kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.