-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check: replace test runner logic with Go's stdlib subtest support #122
base: v1
Are you sure you want to change the base?
Conversation
5b26438
to
f8837e9
Compare
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. |
4a5a9f7
to
24642f5
Compare
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 golang/go#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:
There's definitely room for some improvement here, with some duplication between the stdlib line prefixes and gocheck output. |
14d13ea
to
2ac1ab7
Compare
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. |
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. |
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:
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. |
0de55d5
to
ec280f8
Compare
7952b88
to
e46c731
Compare
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 I ended up removing almost all of the go-check command line flags except for |
Also, don't print file name/line since stdlib logging does that for us.
The new testing strategy is to run the closure with a fake testing.TB to collect logs and test success/failure information.
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:
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:testing.T
:Error
,Errorf
,Fatal
,Fatalf
,Log
, andLogf
Fail
andFailNow
Failed
Skip
: slightly different signature, but compatibleAssert
andCheck
MkDir
: now wrapsT.TempDir
, new in Go 1.15Output
TestName
: now wrapsT.Name
N
ResetTimer
,StartTimer
, andStopTimer
SetBytes
ExpectFailure
,Succeed
, andSucceedNow
: stdlib testing does not support undoing a test failureGetTestLog
: stdlib testing does not exposeThe 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:
Parallel()
to allow tests in a suite to run in parallel. Of course, this isn't necessarily safe for tests usingSetUpTest
/TearDownTest
, since each test is sharing the same instance of the suite. CallingParallel
fromSetUpSuite
could be used to parallelise multiple suites though.testing.T
can be used within go-check tests.We lose a few features too:
Panics
andPanicMatches
checkers still work as before to test expected panics though.C.Succeed
,C.SucceedNow
, andC.ExpectFailure
can't be implemented.