Refactor tests to use testify require library

[miketonks-form3]

Refactor test to make code cleaner and easier to read, as baseline for future improvements. The testify library provides excellent helper functions for test assertions, let's use them.

Work towards of investment time project: https://github.com/form3tech/investment-time-project/issues/62

Co-authored-by: Andrea Rosa [email protected]
Co-authored-by: Kevin Intriago [email protected]

[ghost]

Hey. I came here because I really dislike assertion libraries and was curious about this pull request. The reasons I dislike are here: https://go.dev/doc/faq#assertions, but this isn't exactly why I'm writing this comment.

The thing that concerned me is that I see that testify/require was used, and it calls t.FailNow (as in, t.Fatal).
Using t.Fatal instead of t.Error on concurrent code messes up the error propagation, and I guess probably wants to be avoided in testing code for testing something concurrent: https://go.dev/doc/go1.16#vet-testing-T

The vet tool now warns about invalid calls to the testing.T method Fatal from within a goroutine created during the test. This also warns on calls to Fatalf, FailNow, and Skip{,f,Now} methods on testing.T tests or testing.B benchmarks.

Calls to these methods stop the execution of the created goroutine and not the Test* or Benchmark* function. So these are required to be called by the goroutine running the test or benchmark function.

If you really want to use an assertion library, you might want to consider using another package (like, ../assert) which uses t.Fail instead (as in, t.Error) to make sure you can use your checks inside goroutines concurrently safely.

[miketonks-form3]

Thanks @henrique-pinto-form3 for the feedback. I wasn't aware of this issue and it is always good to learn something new.

It's interesting, perhaps slightly worrying, that go vet can't spot the issue.

I've updated the PR to use assert instead which on reflection is closer to the original behavior.

[ghost]

Using FailNow sometimes make sense. Not as common, but not a defect, so it wouldn't make sense for go vet to report it. Static analyzers can only do so much!

[ryangough-f3]

There was a general comment about calling FailNow from @henrique-pinto-form3 . Doesn't this method still potentially end up calling t.FailNow under the hood?

[sata-form3]

I don't think this is a problem, we're not creating a goroutine as part of the stage setup?

[miketonks-form3]

I intended to change all to assert, mainly for consistence, just overlooked this file. Will change.