Unify tests 🧟 👻

[husio]

:information_source: This is a huge issue that everyone is welcome to work on. Pick a package that was not yet updated and make a pull request.

Do not assign this issue to yourself and do not move it to In progress unless you intend to fix all packages.

---

We have decided to unify all tests and use only standard library testing package with a small number of test helpers that we write and maintain ourselves.

This task is to gradually (package at a time)

• remove all testify package usage and use our weavetest/assert package instead, or just testing
• remove all convey package usage
• change table tests cases declaration from using slice of structs to use map[string]struct{}

Packages to be updated

• [ ] cmd
• [ ] cmd/bnsd
• [x] cmd/validators
• [x] cmd/lsaddr
• [x] cmd/bcpd
• [ ] tmtest
• [x] scripts
• [x] errors
• [x] store
• [x] store/iavl
• [x] coin
• [x] orm
• [x] app
• [x] app/testdata
• [x] gconf
• [x] crypto
• [x] crypto/bech32
• [x] x
• [x] x/msgfee
• [x] x/escrow
• [x] x/cash
• [x] x/multisig
• [x] x/currency
• [x] x/batch
• [x] x/validators
• [x] x/nft
• [x] x/hashlock
• [x] x/paychan
• [x] x/distribution
• [x] x/sigs
• [x] x/namecoin
• [x] x/utils

[ethanfrey]

We had some discussions between stdlib and testify... Basically whether the arguably less typing of testify was worth the complexity of a library.

I stumbled across this, which might be a middle-ground idea... https://github.com/benbjohnson/testing

Basically, just create 3-4 small custom helper functions that we can use for common idioms on top of stdlib but not requiring the whole testify package.

Of course, if this grows into 20 helper functions, we defeat the whole purpose... so just a few well-chosen ones, like failIfError()

[husio]

I did not know about testing.TB! :tada:

I like that he encurages to copy the code rather than use his package. 3 points for Ben Johnson!

I do not like that those functions

• use terminal colors which not everyone may like and is most likely broken on CI (there are better soultions)
• use runtime.Caller instead of t.Helper(). Let the testing package do its job!
• are just an alias to !, != and reflect.DeepEqual. You have to lookup what ok or equals does and err != nil and reflect.DeepEqual(want, got) is obvious.
• they are not flexible, because they cover just some cases. What if I want to make sure I got an error. Do I define notOk?

---

I have tried to use testify and I am still not convinced. To keep it dense here are the key points

• using testify requires you to learn a big API, (just look at require package :see_no_evil: ). I am not surprised that sometimes it is used wrong.
• using testify does not require any less typing

  
  // 56 characters (including whitespace)
  require.NoError(t, err, "expected success, got %v", err)
  // 59 characters (including whitespace) 
  if err != nil { t.Fatalf("expected success, got %v", err) }

// 70 characters require.True(t, errors.IsNotFound(err), "want not found, got %v", err) // 70 characters if !errors.IsNotFound(err) { t.Fatalf("want not found, got %s", err) }

- when using testify failure messages are left empty which is bad :no_good_man: It is too easy to type `require.Nil(t, err)` and call it a day. Instead it should be `require.NoError(t, err, "cannot do XXX while YYY")`.

[ethanfrey]

Ha, the point is you don't let testify handle the error messages for you. Basically, the auto-generated error messages are often quite good...

replace require.NoError(t, err, "expected success, got %v", err) with require.NoError(t, err) and it will do the same, but with %+v to give full stack-trace if available.

also, require.Equal(t, a, b) doesn't need a "expected %v, got %v" parameter, as it will auto-generate nice diff on error (like high-lighting just the part of the json that was different on large inputs).

[ethanfrey]

    when using testify failure messages are left empty which is bad no_good_man It is too easy to type require.Nil(t, err) and call it a day. Instead it should be require.NoError(t, err, "cannot do XXX while YYY").

I disagree, as for tests, the line number and a good dump of the variables should explain what needs to be done. Oh, and don't require.Nil(t, err), it is always better require.NoError(t, err), both for clarity and better error output

[ethanfrey]

I do not like that those functions

use terminal colors which not everyone may like and is most likely broken on CI (there are better soultions) ...

I agree, I would not use them as-is. But the idea behind them, to use a handful of functions rather than a framework is interesting to me.

As I said, require.NoError(t, err) is very nice when I just want to ensure nothing errored, without an if statement for every line

and assert.Equals(t, a, b) where it shows a proper diff when they are different (rather than the repetative fmt string each time) is a nice simplifier.

assert.True/False really should have better output strings with them, so there is not much savings

Maybe there are a few more, but I bet making our own helper function for those two cases would remove 80% of the times when I wish I had testify over stdlib

[husio]

This remind me a bit of pytest vs unittest in Python.

Unittest library provides a lot of methods, almost a separate method for each case (aka testify). Pytest relies on only assert statement leaving the code to the user. Ok, this is not a fair comparison because pytest is using magic to make its reports great :godmode:

I do not think we will convince each other. I guess this is purely preference choice now. Some enjoy using vim wile others choose to suffer emacs :smiling_imp:

[ruseinov]

I guess the only thing I'd really be missing is the ability to use testify mocks, but otherwise ok. Also easier for 3rd party devs to contribute, when there are no 3rd party libs.

[kmw101]

🎢 Finished....

[ruseinov]

@kmw101 why is this finished? There's a couple more I think

[ruseinov]

lazyretina-tb:weave romanuseinov$ git grep testify
app/chain_test.go:      "github.com/stretchr/testify/assert"
cmd/bnsd/app/app_test.go:       "github.com/stretchr/testify/assert"
cmd/bnsd/app/app_test.go:       "github.com/stretchr/testify/require"
cmd/bnsd/app/init_test.go:      "github.com/stretchr/testify/assert"
cmd/bnsd/app/init_test.go:      "github.com/stretchr/testify/require"
cmd/bnsd/client/client_test.go: "github.com/stretchr/testify/assert"
cmd/bnsd/client/client_test.go: "github.com/stretchr/testify/require"
cmd/bnsd/client/keys_test.go:   "github.com/stretchr/testify/assert"
cmd/bnsd/client/keys_test.go:   "github.com/stretchr/testify/require"
cmd/bnsd/client/tx_test.go:     "github.com/stretchr/testify/assert"
cmd/bnsd/client/tx_test.go:     "github.com/stretchr/testify/require"
cmd/bnsd/client/wallet_test.go: "github.com/stretchr/testify/assert"
cmd/bnsd/client/wallet_test.go: "github.com/stretchr/testify/require"
cmd/bnsd/commands/init_test.go: "github.com/stretchr/testify/assert"
cmd/bnsd/commands/init_test.go: "github.com/stretchr/testify/require"
cmd/bnsd/commands/start_test.go:        "github.com/stretchr/testify/require"
cmd/bnsd/scenarios/escrow_test.go:      "github.com/stretchr/testify/assert"
cmd/bnsd/scenarios/escrow_test.go:      "github.com/stretchr/testify/require"
cmd/bnsd/scenarios/validator_test.go:   "github.com/stretchr/testify/assert"
cmd/bnsd/scenarios/validator_test.go:   "github.com/stretchr/testify/require"
cmd/bnsd/scenarios/wallet_test.go:      "github.com/stretchr/testify/require"
coin/coins_test.go:     "github.com/stretchr/testify/assert"
coin/coins_test.go:     "github.com/stretchr/testify/require"
go.mod: github.com/stretchr/testify v1.3.0
go.sum:github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
go.sum:github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
orm/index_test.go:      "github.com/stretchr/testify/assert"
orm/index_test.go:      "github.com/stretchr/testify/require"
orm/multiref_test.go:   "github.com/stretchr/testify/assert"
orm/multiref_test.go:   "github.com/stretchr/testify/require"
orm/object_test.go:     "github.com/stretchr/testify/assert"
orm/object_test.go:     "github.com/stretchr/testify/require"
x/cash/init_test.go:    "github.com/stretchr/testify/assert"
x/cash/init_test.go:    "github.com/stretchr/testify/require"
x/cash/staticfee_test.go:       "github.com/stretchr/testify/assert"
x/cash/staticfee_test.go:       "github.com/stretchr/testify/require"
x/gov/init_test.go:     "github.com/stretchr/testify/require"

[ruseinov]

I intend to finish this off

[ethanfrey]

You are true heroes!

Saving weave from the zombies