fix: enable formatter rule from testifylint #18741

ghouscht · 2024-10-15T12:48:05Z

This PR enables the formatter testifylint rule as part of #18719

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

k8s-ci-robot · 2024-10-15T12:48:14Z

Hi @ghouscht. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov-commenter · 2024-10-15T13:06:16Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.82%. Comparing base (fa89ee0) to head (10f76ac).
Report is 8 commits behind head on main.

❗ Current head 10f76ac differs from pull request most recent head be02cdb

Please upload reports for the commit be02cdb to get more accurate results.

Files with missing lines	Patch %	Lines
etcdctl/ctlv3/command/ep_command.go	0.00%	6 Missing ⚠️
client/v3/config.go	85.71%	1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/storage/mvcc/testutil/hash.go	`83.78% <100.00%> (ø)`
client/v3/config.go	`84.90% <85.71%> (+0.29%)`	⬆️
etcdctl/ctlv3/command/ep_command.go	`0.00% <0.00%> (ø)`

... and 23 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18741      +/-   ##
==========================================
+ Coverage   68.73%   68.82%   +0.08%     
==========================================
  Files         420      420              
  Lines       35488    35481       -7     
==========================================
+ Hits        24392    24419      +27     
+ Misses       9661     9635      -26     
+ Partials     1435     1427       -8

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa89ee0...be02cdb. Read the comment docs.

ghouscht · 2024-10-15T13:06:34Z

client/pkg/testutil/assert.go

@@ -40,12 +40,12 @@ func AssertNotNil(t *testing.T, v any) {
 // Deprecated: use github.com/stretchr/testify/assert.True instead.
 func AssertTrue(t *testing.T, v bool, msg ...string) {
 	t.Helper()
-	assert.True(t, v, msg)
+	assert.True(t, v, msg) //nolint:testifylint


Both the AssertTrue and AssertFalse functions are part of the public API of the testutils pkg. It looks like they are no longer used within the etcd codebase but they could be used outside of the etcd codebase. I thought it is best to keep them as they are, thus the nolint directive. Let me know if you think that is ok.

Until this code is removed, it's better to keep it as is . I agree to disable the linter here

ghouscht · 2024-10-15T13:08:20Z

FYI: @mmorel-35 here is the second PR for the formatterrule. From my point of view it is ready but I marked it as draft to keep the number of open and ready testifylint PRs small.

mmorel-35 · 2024-10-15T16:56:40Z

There are many places where the expected or unexpected error is is printed with : %v", err

I'm wondering if it's not redondant with the behavior of testify which will print it

server/etcdserver/api/v2store/stats_test.go

ghouscht · 2024-10-16T06:05:53Z

There are many places where the expected or unexpected error is is printed with : %v", err

I'm wondering if it's not redondant with the behavior of testify which will print it

Yes, that is true. I can clean them up if you and the maintainers agree on that.

mmorel-35 · 2024-10-16T06:28:37Z

Yes, that is true. I can clean them up if you and the maintainers agree on that.

I feel like it's the best opportunity to do that as most of them are probably modified in this PR.

Let's see what the maintainers think.

ahrtr · 2024-10-16T08:58:36Z

tests/e2e/corrupt_test.go


 	leaderIndex := epc.WaitLeader(t)
 	_, err = epc.Procs[leaderIndex].Logs().ExpectWithContext(ctx, expect.ExpectedResponse{Value: "finished compaction hash check"})
-	require.NoError(t, err, "can't get log indicating finished compaction hash check")
+	require.NoErrorf(t, err, "can't get log indicating finished compaction hash check")


Question: there is no variable-length variable in this case, shouldn't require.NoError be better than require.NoErrorf?

Note this is a generic question.

I had the same question and the author of linter directed me to the rule's documentation:

https://github.com/Antonboom/testifylint?tab=readme-ov-file#formatter

And the historical note associated:

https://github.com/Antonboom/testifylint?tab=readme-ov-file#historical-reference

We should always use *f functions when require-f-funcs: true no matter there is variable-length variables or not?

That seems to be the recommended way to do indeed

ahrtr · 2024-10-16T08:59:09Z

tools/.golangci.yaml

      - go-require
      - require-error
    enable-all: true
+    formatter:
+      require-f-funcs: true


What does this option mean? Could you add a comment to clarify?

https://golangci-lint.run/usage/linters/#testifylint states:

To require f-assertions if format string is used.

I'll add a comment 👍🏻

ahrtr · 2024-10-16T09:07:00Z

There are many places where the expected or unexpected error is is printed with : %v", err

I'm wondering if it's not redondant with the behavior of testify which will print it

Yes, testify already prints the error, so we don't need to print it again. But not a big deal.

Signed-off-by: Thomas Gosteli <thomas.gosteli@protonmail.ch>

ghouscht · 2024-10-16T12:23:50Z

There are many places where the expected or unexpected error is is printed with : %v", err
I'm wondering if it's not redondant with the behavior of testify which will print it

Yes, testify already prints the error, so we don't need to print it again. But not a big deal.

👍🏻 I cleaned up the messages and removed redundant information.

k8s-ci-robot · 2024-10-16T13:57:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ghouscht, mmorel-35
Once this PR has been reviewed and has the lgtm label, please assign wenjiaswe for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mmorel-35 · 2024-10-16T17:33:05Z

@ghouscht ,
Feel free to undraft when you you are ready

k8s-ci-robot added do-not-merge/work-in-progress area/clientv3 area/robustness-testing area/testing area/tooling needs-ok-to-test labels Oct 15, 2024

k8s-ci-robot added the size/L label Oct 15, 2024

ghouscht commented Oct 15, 2024

View reviewed changes

ghouscht mentioned this pull request Oct 15, 2024

Enable testifylint linter rules #18719

Open

13 tasks

mmorel-35 reviewed Oct 15, 2024

View reviewed changes

server/etcdserver/api/v2store/stats_test.go Show resolved Hide resolved

ghouscht mentioned this pull request Oct 16, 2024

formatter: remove empty message string in assert.Equalf conversions (autofix) Antonboom/testifylint#194

Open

ahrtr reviewed Oct 16, 2024

View reviewed changes

ghouscht force-pushed the testifylint-formatter branch 2 times, most recently from 4babfc2 to 2d144f0 Compare October 16, 2024 12:18

k8s-ci-robot added the github_actions Pull requests that update GitHub Actions code label Oct 16, 2024

ghouscht force-pushed the testifylint-formatter branch from 2d144f0 to 76e0c60 Compare October 16, 2024 12:19

fix: enable formatter rule from testifylint

be02cdb

Signed-off-by: Thomas Gosteli <thomas.gosteli@protonmail.ch>

ghouscht force-pushed the testifylint-formatter branch from 76e0c60 to be02cdb Compare October 16, 2024 12:20

mmorel-35 approved these changes Oct 16, 2024

View reviewed changes

ghouscht marked this pull request as ready for review October 16, 2024 17:33

k8s-ci-robot removed the do-not-merge/work-in-progress label Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enable formatter rule from testifylint #18741

fix: enable formatter rule from testifylint #18741

ghouscht commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

codecov-commenter commented Oct 15, 2024 •

edited

Loading

ghouscht Oct 15, 2024

mmorel-35 Oct 15, 2024

ghouscht commented Oct 15, 2024

mmorel-35 commented Oct 15, 2024

ghouscht commented Oct 16, 2024

mmorel-35 commented Oct 16, 2024

ahrtr Oct 16, 2024

ahrtr Oct 16, 2024

mmorel-35 Oct 16, 2024

ahrtr Oct 16, 2024

mmorel-35 Oct 16, 2024

ahrtr Oct 16, 2024

ghouscht Oct 16, 2024

ahrtr commented Oct 16, 2024

ghouscht commented Oct 16, 2024

k8s-ci-robot commented Oct 16, 2024

mmorel-35 commented Oct 16, 2024

fix: enable formatter rule from testifylint #18741

Are you sure you want to change the base?

fix: enable formatter rule from testifylint #18741

Conversation

ghouscht commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

codecov-commenter commented Oct 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghouscht commented Oct 15, 2024

mmorel-35 commented Oct 15, 2024

ghouscht commented Oct 16, 2024

mmorel-35 commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Oct 16, 2024

ghouscht commented Oct 16, 2024

k8s-ci-robot commented Oct 16, 2024

mmorel-35 commented Oct 16, 2024

codecov-commenter commented Oct 15, 2024 •

edited

Loading