Skip to content
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.

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

Use jaekwon/testify or stretchr/testify #777

Closed
tbruyelle opened this issue Apr 25, 2023 · 5 comments · Fixed by #1931
Closed

Use jaekwon/testify or stretchr/testify #777

tbruyelle opened this issue Apr 25, 2023 · 5 comments · Fixed by #1931

Comments

@tbruyelle
Copy link
Contributor

Codebase currently uses both of them, which is confusing because if I'm not wrong the only difference between the 2 is the expected vs actual order in methods.

@moul
Copy link
Member

moul commented Apr 26, 2023

Regarding the .go part, I'll defer to @jaekwon.

In my opinion, I feel like I'm missing an equivalent in .gno and feel uncomfortable writing tests without it. Therefore, I propose porting the final choice to Gno and ensuring a fair approach to licensing/copyright (related to issue #679).

@jaekwon
Copy link
Contributor

jaekwon commented Jul 12, 2023

Let's go with the jaekwon one. We can just fork it over to gnolang/testify and use that.
And yes, porting to gno sounds good too.
Ideally lean too, if reasonable.

Please reassign if you need my input.

@jaekwon jaekwon removed their assignment Jul 12, 2023
@moul
Copy link
Member

moul commented Jul 13, 2023

FYI, I started experimenting in Gno with an even simpler API not relying on reflection: #928.

@thehowl
Copy link
Member

thehowl commented Jul 13, 2023

I personally always found testify to be useful but often with too many features for what we generally need for tests. Here's something to prove my point:

$ rg --iglob '*_test.go' -o '(assert|require)\.[A-Za-z0-9_]*' | cut -d: -f2 | cut -d. -f2 | sort | uniq -c | sort -rn
    997 Equal
    622 NoError
    471 Nil
    318 True
    169 NotNil
    150 False
    149 EqualValues
    107 Error
     90 Panics
     63 New
     36 Contains
     34 Len
     31 NotEqual
     24 Empty
     19 NotEmpty
     17 Zero
     16 Fail
     15 NotPanics
      6 IsType
      5 NotZero
      5 NotContains
      5 Exactly
      4 Regexp
      4 EqualError
      2 JSONEq
      2 ErrorIs
      2 Errorf
      1 NoErrorf
      1 InDelta
      1 Greater
      1 ErrorContains
      1 Equalf

In another project we had made a reduced assertions package with only 18 assertion functions. I thought it was useful as having less functions means gopls is snappier and the autocompletions tend to be more around the functions that are actually used.

So if we're deviating from stretchr/testify (I understand the current fork is to swap expected/actual, which always bothered me as well), we might also have it as a reduced subset we maintain in a small package in the repo.

As for the Gno counterpart, I think a reduced subset makes sense. However, Equal/NotEqual, which are undoubtedly the most used assertions, rely on reflect.DeepEqual to work on complex types. Maybe that could be a good first function to implement in an eventual reflect package?

@tbruyelle
Copy link
Contributor Author

Note that jaekwon/testify is not up to date and it's missing some methods like ErrorIs

@moul moul moved this to 🔵 Not Needed for Launch in 🚀 The Launch [DEPRECATED] Sep 5, 2023
@moul moul added this to the 💡Someday/Maybe milestone Sep 6, 2023
thehowl pushed a commit that referenced this issue Apr 18, 2024
Currently, we use 2 implementations of testify in our codebase -
`jaekwon/testify` and `stretchr/testify`

To streamline our practices and minimize confusion, it's a good idea to
opt for a single implementation.

After discussion, we all agreed on using `stretchr/testify` throughout
our codebase.

Closes #777
omarsy pushed a commit to TERITORI/gno that referenced this issue Apr 21, 2024
Currently, we use 2 implementations of testify in our codebase -
`jaekwon/testify` and `stretchr/testify`

To streamline our practices and minimize confusion, it's a good idea to
opt for a single implementation.

After discussion, we all agreed on using `stretchr/testify` throughout
our codebase.

Closes gnolang#777
zivkovicmilos pushed a commit that referenced this issue Jul 2, 2024
Addresses #923 
Addresses #777

---------

Signed-off-by: Manfred Touron <[email protected]>
Signed-off-by: moul <[email protected]>
gfanton pushed a commit to gfanton/gno that referenced this issue Jul 23, 2024
Addresses gnolang#923 
Addresses gnolang#777

---------

Signed-off-by: Manfred Touron <[email protected]>
Signed-off-by: moul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔵 Not Needed for Launch
Development

Successfully merging a pull request may close this issue.

4 participants