etcd

etcd copied to clipboard

This PR enables the formatter testifylint rule as part of https://github.com/etcd-io/etcd/issues/18719

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

Oct 15 '24 12:10 ghouscht

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.

Oct 15 '24 12:10 k8s-ci-robot

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 68.71%. Comparing base (3d6ff97) to head (8495e8d). :warning: Report is 2054 commits behind head on main.

:warning: Current head 8495e8d differs from pull request most recent head ff4a8df

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

@@            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     

Oct 15 '24 13:10 codecov-commenter

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.

Oct 15 '24 13:10 ghouscht

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

Oct 15 '24 16:10 mmorel-35

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.

Oct 16 '24 06:10 ghouscht

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.

Oct 16 '24 06:10 mmorel-35

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.

Oct 16 '24 09:10 ahrtr

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.

Oct 16 '24 12:10 ghouscht

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

Oct 16 '24 17:10 mmorel-35

/ok-to-test

Oct 17 '24 04:10 ivanvc

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

Oct 17 '24 05:10 mmorel-35

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

Oct 17 '24 12:10 mmorel-35

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

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

Oct 18 '24 06:10 ghouscht

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.

Oct 18 '24 12:10 ahrtr

[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

Oct 21 '24 22:10 k8s-ci-robot

Raised a followup https://github.com/etcd-io/etcd/pull/18766

Oct 22 '24 05:10 ahrtr

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:

1. https://github.com/Antonboom/testifylint?tab=readme-ov-file#3
2. https://github.com/golangci/golangci-lint/pull/5131

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:

• https://github.com/Antonboom/testifylint/pull/200
• https://github.com/stretchr/testify/issues/1679
• https://github.com/Antonboom/testifylint/issues/201
• https://github.com/Antonboom/testifylint/issues/202

Thanks! 😭

Nov 13 '24 21:11 Antonboom