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

All modified and coverable lines are covered by tests ✅

Project coverage is 68.71%. Comparing base (3d6ff97) to head (8495e8d).

❗ Current head 8495e8d differs from pull request most recent head ff4a8df

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

❗ 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%> (ø)`

... and 27 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18741      +/-   ##
==========================================
- Coverage   68.77%   68.71%   -0.06%     
==========================================
  Files         420      420              
  Lines       35501    35501              
==========================================
- Hits        24416    24395      -21     
- Misses       9657     9675      +18     
- Partials     1428     1431       +3

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 3d6ff97...ff4a8df. 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.

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.

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

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

ivanvc · 2024-10-17T04:30:36Z

/ok-to-test

mmorel-35 · 2024-10-17T05:02:16Z

/test pull-etcd-integration-2-cpu-arm64

ivanvc · 2024-10-17T05:39:15Z

tools/.golangci.yaml

      - go-require
      - require-error
    enable-all: true
+    formatter:
+      # Require f-assertions (e.g. assert.Equalf) if a format string is used.


It could be just a nitpick on the comment, but after reading

this conversation fix: enable formatter rule from testifylint #18741 (comment)

and the references https://github.com/Antonboom/testifylint?tab=readme-ov-file#formatter, https://github.com/Antonboom/testifylint?tab=readme-ov-file#historical-reference.

I think this comment's "format string" part is inaccurate. The suggestion is to use the *f functions regardless of whether the message string has formatting or not (if I understood correctly).

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

The comment is:

To require f-assertions if format string is used.

You're right, even in the documentation from golangci-lint it is not very accurate. I think it something like

Require f-assertions (e.g. assert.Equalf) if a message is passed to the assertion.

might be better.

How about

Require f-assertions (e.g. assert.Equalf) if a message is passed to the assertion, even there is no variable-length variables, i.e. require require.NoErrorf for both cases below,
require.NoErrorf(t, err, "whatever message")
require.NoErrorf(t, err, "whatever message: %v", v)

Signed-off-by: Thomas Gosteli <[email protected]>

mmorel-35 · 2024-10-17T12:40:51Z

/test pull-etcd-integration-1-cpu-amd64

ghouscht · 2024-10-18T06:53:07Z

/test pull-etcd-integration-1-cpu-amd64

One last try, seems there is always some kind of timeout in this test.

ahrtr · 2024-10-18T12:45:32Z

From golang programming perspective, I think we still prefer non-f-functions (i.e. fmt.Print) to f-functions (i.e. fmt.Printf) when there is no variable-length parameters.

For example, we prefer to use Print instead of Printf when no variable-length parameters.

in fmt package

func Print(a ...any) (n int, err error)
func Printf(format string, a ...any) (n int, err error)

in log package

func (l *Logger) Print(v ...any)
func (l *Logger) Printf(format string, v ...any)

But in stretchr/testify, each f-function is actually a syntax sugar of corresponding non-f-function. For example, Containsf is just a simple encapsulation of Contains. also refer to here.

So there is no reason to keep both. But they can't simplify remove anyone otherwise it will break lots of application's tests.

func Containsf(t TestingT, s interface{}, contains interface{}, msg string, args ...interface{}) bool {
	if h, ok := t.(tHelper); ok {
		h.Helper()
	}
	return Contains(t, s, contains, append([]interface{}{msg}, args...)...)
}

So it's accepted to always require f-functions for stretchr/testify, but not for golang standard lib.

ahrtr

LGTM

Thanks

ivanvc

To @ahrtr's point, I agree it may not be straight forward if you're not familiar with stretchr/testify. When I initially saw the pull request I had to check the documentation to understand the issue and the linter rule.

But with that aside, it LGTM. Thanks, @ghouscht.

k8s-ci-robot · 2024-10-21T22:07:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ghouscht, ivanvc, mmorel-35

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:

~~OWNERS~~ [ahrtr]

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

ahrtr · 2024-10-22T05:25:34Z

Raised a followup #18766

Antonboom · 2024-11-13T21:15:54Z

Hi, guys

@ghouscht appeciated for using of testifylint. @mmorel-35, thank you for support.

@ahrtr, @ivanvc I improved the linter description based on your threads:

Also, I reviewed the testify code deeper and was surprised, but you can use non-string arg in asserts if this is the single argument, like

assert.Equal(a, b, new(time.Time))

It leads to the following points:

Thanks! 😭

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

Closed

19 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 2 times, most recently 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

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 17, 2024

ivanvc reviewed Oct 17, 2024

View reviewed changes

ghouscht force-pushed the testifylint-formatter branch from be02cdb to 4af1959 Compare October 17, 2024 12:09

fix: enable formatter rule from testifylint

ff4a8df

Signed-off-by: Thomas Gosteli <[email protected]>

ghouscht force-pushed the testifylint-formatter branch from 4af1959 to ff4a8df Compare October 17, 2024 12:10

ahrtr approved these changes Oct 18, 2024

View reviewed changes

k8s-ci-robot added the approved label Oct 18, 2024

ivanvc approved these changes Oct 21, 2024

View reviewed changes

ahrtr merged commit a7dcfdb into etcd-io:main Oct 22, 2024
33 checks passed

ahrtr mentioned this pull request Oct 22, 2024

Add comment to clarify non-f-functions on golang standard lib #18766

Merged

ghouscht deleted the testifylint-formatter branch November 7, 2024 08:25

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

mmorel-35 commented Oct 16, 2024

ivanvc commented Oct 17, 2024

mmorel-35 commented Oct 17, 2024

ivanvc Oct 17, 2024

mmorel-35 Oct 17, 2024 •

edited

Loading

ghouscht Oct 17, 2024 •

edited

Loading

ahrtr Oct 17, 2024

mmorel-35 commented Oct 17, 2024

ghouscht commented Oct 18, 2024

ahrtr commented Oct 18, 2024

ahrtr left a comment

ivanvc left a comment

k8s-ci-robot commented Oct 21, 2024

ahrtr commented Oct 22, 2024

Antonboom commented Nov 13, 2024

fix: enable formatter rule from testifylint #18741

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

mmorel-35 commented Oct 16, 2024

ivanvc commented Oct 17, 2024

mmorel-35 commented Oct 17, 2024

Choose a reason for hiding this comment

mmorel-35 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

ghouscht Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorel-35 commented Oct 17, 2024

ghouscht commented Oct 18, 2024

ahrtr commented Oct 18, 2024

ahrtr left a comment

Choose a reason for hiding this comment

ivanvc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 21, 2024

ahrtr commented Oct 22, 2024

Antonboom commented Nov 13, 2024

codecov-commenter commented Oct 15, 2024 •

edited

Loading

mmorel-35 Oct 17, 2024 •

edited

Loading

ghouscht Oct 17, 2024 •

edited

Loading