frankban / quicktest

Quick helpers for testing Go applications
MIT License
529 stars 27 forks source link

Consider API redesign with the current testing package #75

Closed mvdan closed 2 years ago

mvdan commented 4 years ago

I realise that this module is a v1 and we cannot break backwards compatibility, which is a good thing. However, the testing package has evolved quite a bit in the past few years - especially with methods like TempDir and Cleanup, which came directly from this module :)

I'm starting to wonder if designing a v2 would be a good idea. The simplest changes we could make are removing deprecated APIs now available in the upstream package, such as Mkdir, Defer, and Done. I imagine there might also be some other tech debt that we could fix when given the chance to break backwards compatibility.

I also wonder if we could remove the New constructor entirely, and replace methods like C.Check with qt.Check. For example, instead of:

func TestFoo(t *testing.T) {
    c := qt.New(t)
    numbers, err := somepackage.Numbers()
    c.Assert(numbers, qt.DeepEquals, []int{42, 47})
    c.Assert(err, qt.ErrorMatches, "bad wolf")
}

We could just do:

func TestFoo(t *testing.T) {
    numbers, err := somepackage.Numbers()
    qt.Assert(t, numbers, qt.DeepEquals, []int{42, 47})
    qt.Assert(t, err, qt.ErrorMatches, "bad wolf")
}

Presumably, part of the reason a state was needed was for Defer/Done, which should no longer apply. It seems like the only other reason to keep state around is C.SetFormat. I hope that we have alternative options there, such as a global per-package format, or deriving the format from the test name, or globally registering a format for a specific *testing.T like qt.SetFormat(t, ...).

/cc @rogpeppe who I think has been thinking about a possible v2 as well

rogpeppe commented 4 years ago

I think it's worth keeping the current API around as we don't really want people to have to change all their code to move to the new API and the old idiom involves less typing. But it's not exclusive. We could do something like this:

package qt
func Assert(t testing.TB, etc)
func Check(t testing.TB, etc)
type C struct {
    testing.TB
}
func New(t testing.TB) *C {
    return &C{t}
}
func (c *C) Assert(etc) {
    return Assert(c.TB, ...)
}
func (c *C) Check(etc) {
    return Check(c.TB, ...)
}
frankban commented 4 years ago

This is interesting, thanks for the input!

I think it's still worth keeping Mkdir, Defer and Done for now, as they are useful when having to test a code base against multiple versions of Go, like in the library use case, or when having to migrate code to Go >= 1.15 before being able to leverage the new features of the testing package. What do you think?

I agree with Roger that we can have both top level Assert, Check, Patch, Setenv, Unsetenv functions and a C wrapper for the old idiom. Could you please expand a bit on the reason this new idiom would be more compelling in your experience? I guess one reason is that, when you just need a quick assertion on your test, calling a function is quicker than getting a *C instance and then assert in two separate lines.

mvdan commented 4 years ago

Keeping deprecated APIs around for a few extra Go releases makes sense. I intended such breaking changes to be aimed towards 2021 or so :) Roger's idea to simply improve v1 for now also makes sense.

when you just need a quick assertion on your test, calling a function is quicker than getting a *C instance and then assert in two separate lines.

Indeed - for short/small tests, avoiding the extra line and variable feels better. It also feels like you're just using a library as opposed to a testing framework, as you're not wrapping nor replacing t in any way.

On the other hand, for larger or more complex tests, I see how C can be useful. In a way, this choice between simplicity and scalability reminds me of testing.T.Run for subtests; it doesn't make sense for small tests, but for larger stuff, it really does help. But both options (normal tests and many sub-tests) are options when writing test functions.

rogpeppe commented 4 years ago

If we do a v2, then I think we could happily drop support for old Go releases and streamline the API a lot.

frankban commented 4 years ago

Ok this is now released as v1.11.0.

mvdan commented 4 years ago

Hmm, why was this closed? This is about a v2 redesign, so by definition it's still a research project.

frankban commented 4 years ago

Closed by the merged pull request that implements the new API. But it's fair that this can be used for further v2 API discussion.

mvdan commented 3 years ago

I've also raised https://github.com/frankban/quicktest/issues/90 about potentially using a simpler package name/path.

frankban commented 2 years ago

v2 for quicktest is essentially https://github.com/go-quicktest/qt That includes an API redesign that takes advantage of compile time checks thanks to Go generics. As no backward compatible constraints are in place, that also drops the functionality (Defer, Done, Cleanup, MkDir) that over time has been included in the stdlib. I'd close this in favor of https://github.com/go-quicktest/qt/issues/7 for any discussion around the new API.