search

go-check / check

Rich testing for the Go language
Other
696 stars 182 forks source link

check: replace test runner logic with Go's stdlib subtest support #122

Open jhenstridge opened 4 years ago

jhenstridge commented 4 years ago

This PR is a bit of an experiment, and probably not suitable to merge as is. It might act as an inspiration for a "version 2" of the package though.

Go's builtin testing framework has gained a number of features since go-check was created, so I wanted to see how much code I could remove from go-check while still maintaining the most important features:

  1. The ability to register test suites with suite-level and test-level setup/teardown routines.
  2. The Checker system for writing concise assertions.

With the subtest support introduced in Go 1.7 and cleanup support introduced in Go 1.14, it is possible to expose each go-check test as an independent test with its own testing.T instance to record failures. That means we don't need our own test result tracking or output logging.

Most of the logic in check.C is gone, since it is now provided by an embedded *testing.T value. Here is the status of the various methods now:

The embedding also exposes all the other testing.T methods, and will automatically expose new methods as they are added in future Go versions.

This gives us a few new features:

  1. go-check tests within a suite can spawn their own subtests.
  2. Tests can call Parallel() to allow tests in a suite to run in parallel. Of course, this isn't necessarily safe for tests using SetUpTest/TearDownTest, since each test is sharing the same instance of the suite. Calling Parallel from SetUpSuite could be used to parallelise multiple suites though.
  3. Tools designed to analyse standard Go test results will work with go-check test suites.
  4. testing tools built on top of testing.T can be used within go-check tests.

We lose a few features too:

  1. I ripped out the benchmark feature. I haven't seen much use of them in practice, and it's probably best to convert them to regular Go testing benchmarks.
  2. The stdlib test runner does not try to recover from panics within tests, so a panic will halt the test run rather than just fail the current test. The Panics and PanicMatches checkers still work as before to test expected panics though.
  3. The stdlib test framework doesn't provide a way to turn a failing test into a success, so C.Succeed, C.SucceedNow, and C.ExpectFailure can't be implemented.
niemeyer commented 3 years ago

Certainly interesting in learning about how this experiment goes.

One thing to keep in mind is that good part of the value of go-check is actually in its nice reporting. If all we want are the checkers, there are other packages out there, a few even inspired by go-check itself, which might be a good option.

jhenstridge commented 3 years ago

Part of the reason for wanting to do this under the Go Check umbrella is so that it might be a more obvious upgrade path for projects already using gocheck. Part of the idea was to design things so new stdlib testing features would automatically become available to gocheck users as they are added, so they don't need to choose between gocheck's convenience and the stdlib's features.

I realise that having good test coverage is important for a change like this, so I filed https://github.com/golang/go/issues/42827 against Go itself about improving the ability to test test tools. One suggestion given on that bug was to reinvoke the test program run specific tests that would normally be skipped and inspect the output, so that's what I'm trying.

I agree that having concise easy to read output is important. This is a sample of what this PR currently generates for a suite with a failing test, both in regular and verbose modes:

$ go test ./sample
--- FAIL: Test (0.00s)
    --- FAIL: Test/sampleSuite (0.00s)
        --- FAIL: Test/sampleSuite/TestTwo (0.00s)
            sample_test.go:22: sample_test.go:22:
            sample_test.go:22:     c.Check(false, Equals, true)
            sample_test.go:22: ... obtained bool = false
            sample_test.go:22: ... expected bool = true
            sample_test.go:22: 
            sample_test.go:23: sample_test.go:23:
            sample_test.go:23:     c.Check("Hello world", Matches, `^Hello$`)
            sample_test.go:23: ... value string = "Hello world"
            sample_test.go:23: ... regex string = "^Hello$"
            sample_test.go:23: 
FAIL
FAIL    gopkg.in/check.v1/sample    0.010s
FAIL
$ go test -v ./sample
=== RUN   Test
=== RUN   Test/sampleSuite
=== RUN   Test/sampleSuite/TestOne
=== RUN   Test/sampleSuite/TestTwo
    sample_test.go:22: sample_test.go:22:
    sample_test.go:22:     c.Check(false, Equals, true)
    sample_test.go:22: ... obtained bool = false
    sample_test.go:22: ... expected bool = true
    sample_test.go:22: 
    sample_test.go:23: sample_test.go:23:
    sample_test.go:23:     c.Check("Hello world", Matches, `^Hello$`)
    sample_test.go:23: ... value string = "Hello world"
    sample_test.go:23: ... regex string = "^Hello$"
    sample_test.go:23: 
--- FAIL: Test (0.01s)
    --- FAIL: Test/sampleSuite (0.01s)
        --- PASS: Test/sampleSuite/TestOne (0.00s)
        --- FAIL: Test/sampleSuite/TestTwo (0.01s)
FAIL
FAIL    gopkg.in/check.v1/sample    0.013s
FAIL

There's definitely room for some improvement here, with some duplication between the stdlib line prefixes and gocheck output.

christarazi commented 3 years ago

I'd like to say that I support this change. There are many times where I want to run subtests with this framework, look for it and not find it, then remember that the framework doesn't support it. To me, I think it's natural for this library to leverage what the stdlib already does. It made sense to provide a different implementation early on when the stdlib was lacking, but now it doesn't make much sense for there to be "competing" implementations.

dobey commented 1 year ago

Is there any update on this @jhenstridge ? Found this today when looking how to do subtests with gocheck, as I was hoping to use parameterized tests in parallel in a project I'm working on. Would love to see this get merged.

jhenstridge commented 1 year ago

I haven't had time to work on this in a while. I still think this would be a reasonable direction to go for a version 2 of the package:

  1. make the checker infrastructure usable with regular testing.T
  2. turn tests in go-check suites into real stdlib tests. Maybe recommend people use testing.T as the parameter rather than check.C.
  3. make check.C expose all testing.T functionality, so we automatically benefit from new features.
  4. Delete stuff people aren't actively using (benchmarks within go-check suites is the main one), and command line flags if possible.

The main place I got stuck on in this PR was working out what to do with the tests. Unlike the go-check runner, you can't easily inspect the stdlib test runner result. So I'm not sure whether the same level of test coverage would be possible. Maybe that doesn't matter so much though.

jhenstridge commented 5 months ago

I've gone through and fixed up the remainder of the test suite and reworked a few of the changes, so I think this is probably ready for review.

This clearly isn't fully compatible with the existing gopkg.in/check.v1 API, but it is close enough for large code bases like snapd can use it without modification. So if we want to move in this direction it'll probably need to make it v2 of the package.

I ended up removing almost all of the go-check command line flags except for -check.list. I'm half wondering whether it'd be better to drop that too. It's not outputting the test names in the same form the stdlib -test.run argument expects, and the stdlib test runner appears to have decided listing subtests is too hard for now (see https://github.com/golang/go/issues/17209). It they ever do revisit this, it might be easier if we don't have to make it fit with a half-hearted version.