Adopt quicktest?

juju
1

Juju’s unit tests are primarily written with the gocheck library in mind. A recent pull request from @rogpeppe raised the possibility of using quickcheck within Juju code. That PR has been approved by @cmars, but I would like to pause and gain consensus before we take the decision to incorporate it into Juju code.

@manadart has added some detailed thinking into our unit test setup recently:

On quickcheck, Rog has written a good overview of quickcheck. It feels like step forward technically—love the idea of better pretty printing especially—but I’m still wary that another partially-implemented improvement will lead to greater fragmentation in the code base.

2

I certainly like better error/failure formatting. It is a bit of a shame that while it takes the form of “Assert(… Checker)” syntax, it doesn’t maintain any compatibility with gocheck’s actual types. So all of juju/testing/checkers would need to be reworked.
Also, there is the elephant about SetUpTest. Though it seems the pattern is to turn your Suite into a top level Test and then all of your actual tests into c.Run(“subtest” ) calls? Otherwise every one of your Test functions gets these lines:

func TestFoo(t *testing.T) {
  c := qt.New(T)
  defer c.Done()
  c.Defer(setUpSuite(c))
... actual test
}

you probably could put that into a helper to just make it:

func TestFoo(t *testing.T) {
  c := setUpSuite(t)
  defer c.Done()
  ...
}

Alternatively it becomes

func TestSuite(t *testing.T) {
  c := qt.New(t)
  defer c.Done()
  c.Run("test1", testOne)
  c.Run("test2", testTwo)
  c.Run("test3", testThree)
}

Done is automatically called at the end of Run, so you don’t have to repeatedly tear down, but you still need to setUpSuite© at the start of each test.

I can certainly appreciate the idea of Defer(), (in Juju code we had AddCleanup as part of our suites, which does essentially the same thing. Set this up, and then clean it up once the test was done executing, rather than TearDownTest, which is quite unreliable).

As for the other assertion about composition:

// FakeHomeSuite sets up a fake home directory before running tests.
type FakeHomeSuite struct {
    CleanupSuite
    LoggingSuite
    Home *FakeHome
}

There are two things that suites give you, SetUp and extra methods to call. In both cases, I think it is common that you know that you’ll always want both, thus you compose them. For SetUp it is equivalent to:

func setUp(c *gt.C) {
  c.Defer(cleanupSetUp)
  c.Defer(loggingSetUp)
}

rather than repeating that in every test case you write.
For extra methods, imagine you want to set up fixtures, and then use them, and do that across several different tests. If you aren’t using struct composition, then you end up with

func setUp(c *gt.c) (a, b) {
  a := setUpA(c)
  b := setUpB(c)
  return a, b
}

Maybe that’s functionally better, as it makes it clearer what your dependencies are (they are a fixture object you have to use, rather than a side effect of your suite). And probably it is more of a misfeature to depend on 4+ fixtures (where return values get cumbersome).

3

Ensuring that those are right all the time, could be painful if it’s silent. I’m unsure if it’s dependant on compile time vs run time checks.

4

For the record, here’s what I replied in the juju/clock issue:

As to quicktest vs gocheck. The biggest thing is the loss of suite level setup/teardown, though functionally that can be “call SetUp at the start of every test function”.

Two thoughts here:

  1. it’s not always good to have a “suite” type giving common set up for a bunch of tests - it can mean that some tests have more setup overhead than they actually require, and that if you want one test to have a slightly different variant of the setup, it’s hard to do (a test can’t parameterise its own setup).
  2. when you do want a suite, you can use qtsuite which provides similar functionality (and also supports concurrent tests, which is something gocheck can’t do)

The pattern of quicktest Checkers also doesn’t match gocheck. Checkers which means it will be incompatible with juju/testing/checkers. So actually converting a significant portion of the code base is a rather massive undertaking.

I suspect it wouldn’t be too bad. The usage of the checkers is the same, and it’s not hard to make new checkers that behave the same as the old ones. For example, github.com/juju/qthttptest already contains equivalents to some of the testing/checkers checkers (e.g. JSONEquals). Once you’ve made new equivalent checkers, porting the code is just a matter of importing the checkers from a different package.

I changed a bunch of modules from gocheck to quicktest and it was pretty painless.
For the record, these were the edit commands I was using; they got me a lot of the way:

,x s/^func.*\) (Test.*)\(c \*gc.C\) {\n/func \1(t *testing.T) {\n	c := qt.New(t)\n/
,x/gc\./c/qt./
,x/jc.DeepEquals/c/qt.DeepEquals/
,x/qt\.NotNil/c/qt.Not(qt.IsNil)/
,x/jc.ErrorIsNil/c/qt.Equals, nil/

One observation is that the conversion doesn’t have to happen all at once; a code base can happily support both styles of test while migration is happening. For example, in Candid, I made one PR per converted package and it worked pretty well.

Another is that the tests read pretty similarly with both packages, so there’s not a big cognitive gap when jumping between the two, so supporting both styles for a while shouldn’t end up as a huge burden.

5

It won’t compile if you’re using the wrong checker type, so I think you’re OK here.

1 Like
6

To expand on my mention of qtsuite earlier:

func TestAllSuite(t *testing.T) {
    qtsuite.Run(qt.New(t), &mySuite{})
}
type mySuite struct {
   a, b A, B
}
func (s *mySuite) Init(c *qt.C) error {
   // Note that s is a new copy of the value passed to qtsuite.Run, so
   // it's OK to call c.Parallel here to make all the tests in the suite run
   // concurrently.
   a, err := newA()
   c.Assert(err, qt.Equals, nil)
   c.Defer(a.Close)
   c.a = a
   b, err := newB()
   c.Assert(err, qt.Equals, nil)
   c.Defer(b.Close)
   return nil
}
func (s *mySuite) TestOne(c *qt.C) {
    // use s.a and s.b
}
func (s *mySuite) TestTwo(c *qt.C) {
}

Note that this pattern has one specific advantage over gocheck: if something fails in Init, only the relevant Defer functions will be called. This contrasts with gocheck where if something fails in SetUpTest, TearDownTest may be called with a partially filled-out suite struct - this has resulted in some test failures which were quite hard to diagnose.

Also, test fixtures that use Defer are much more easily composable than gocheck “Suite” types. You don’t have to implement any methods to compose two such fixtures - just call each one in turn. No chance of getting the calls to the embedded TearDownTest methods wrong either.