magnusbaeck / logstash-filter-verifier

Apache License 2.0
190 stars 27 forks source link

Align use of test assertions #204

Open magnusbaeck opened 5 days ago

magnusbaeck commented 5 days ago

Right now we make assertions in tests in three different ways; plain old testing.T.Fail and friends, github.com/stretchr/testify/assert (and .../require), and github.com/matryer/is. I think we should use a single package for assertions and use at most one third-party package. I find the standard library's functions very tedious to work with so I really want to get rid of them, and I think github.com/stretchr/testify/assert has a few advantages over github.com/matryer/is (like more assertion functions for comparing data in different ways) but I'm willing to discuss it.

breml commented 3 days ago

I agree, we should settle on a single way to assert in our tests. I also agree, that without assertion package, the tests are tidious to write and me personally, I find them also harder to reason about (in all my clients projects an assertion package is used). In the past, I mainly used github.com/matryer/is, since I liked its very small API surface and for almost all cases, this was enough. But recently, there have been some issues (generics related, if I remember correctly) and I used it less frequently. For me, it is fine to switch to github.com/stretchr/testify/ as the default option. Me personally, I prefer the require flavour, where tests fail fast on the first error, which is the right thing to do in my opinion in most cases (exceptions are, if you e.g. have some sort of "form validation", where you want to report all the differences instead of just the first one).

magnusbaeck commented 3 days ago

Yeah, I frequently use the require variety too. The testifylint linter requires it when you use it for checking for errors (i.e. require.NoError).

I can convert the old code once I'm done with the removal of the old testcase file format.