Skip to content
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

proposal: testing: find and run TestXxx methods on structs that embed *testing.T #12145

Closed
mdwhatcott opened this issue Aug 13, 2015 · 9 comments

Comments

Projects
None yet
6 participants
@mdwhatcott
Copy link

commented Aug 13, 2015

The testing package establishes a simple convention for declaring tests for go test to find and run:

func TestXxx(t *testing.T) {
}

Consider this group of example test functions:

func TestFoo(*testing.T) {
    state = NewSomethingToTest()
    if !state.Foo() {
        t.Error(...)
    }
}

func TestBar(*testing.T) {
    state = NewSomethingToTest()
    if !state.Bar() {
        t.Error(...)
    }
}

func TestBaz(*testing.T) {
    state = NewSomethingToTest()
    if !state.Baz() {
        t.Error(...)
    }
}

Provided all goes well, running the tests produces something akin to the following output:

$ go test -v
=== RUN TestFoo
--- PASS: TestFoo (0.01s)
=== RUN TestBar
--- PASS: TestBar (0.02s)
=== RUN TestBaz
--- PASS: TestBaz (0.03s)
PASS
ok  my/package/name  0.06s

I'd like to see the following convention also allowed:

type MyTests struct {
    *testing.T // embed all the goodness offered by *testing.T (t.Error(), t.Skip, etc...)

    state SomethingToTest
}

// Setup would be invoked before each Test method on *MyTests.
func (t *MyTests) Setup() {
    t.state = NewSomethingToTest()
}

func (t *MyTests) TestFoo() {
    if !t.state.Foo() {
        t.Error(...)
    }
}

func (t *MyTests) TestBar() {
    if !t.state.Bar() {
        t.Error(...)
    }
}

func (t *MyTests) TestBaz() {
    if !t.state.Baz() {
        t.Error(...)
    }
}

Which could produce something like the following output:

$ go test -v
=== RUN Test_MyFixture_Foo
--- PASS: Test_MyFixture_Foo (0.01s)
=== RUN Test_MyFixture_Bar
--- PASS: Test_MyFixture_Bar (0.02s)
=== RUN Test_MyFixture_Baz
--- PASS: Test_MyFixture_Baz (0.03s)
PASS
ok  my/package/name  0.06s

Included in this addition (as seen in the example above) would be optionally defined setup and teardown methods to be run before and after (respectively) each test method. (In an ideal implementation each test method would be invoked on a unique instance of MyFixture which is created just for that test method.)

I've already implemented something very close to this. The only thing I don't like about my implementation is that in order for go test to find and run the test methods on the test structs, I have to run go generate first, which calls a command I've also written as part of that project which generates compatible Test functions that instantiate the struct and then call the test methods (along with defined setups and teardowns). I'd like to be rid of the go generate step to achieve struct-scoped test methods. One benefit here is that you can trivially extract other useful methods on that struct that are called from the test methods. It is trivial because you don't have to pass in any local state to those methods--all the state is already declared on the struct as fields.

Further reading and examples:

Thanks for providing us the testing package and go test, and for your consideration of this idea.

@mikioh mikioh added the Proposal label Aug 13, 2015

@kostya-sh

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2015

Would storing the state in global variables achieve the same effect?

@mdwhatcott

This comment has been minimized.

Copy link
Author

commented Aug 14, 2015

@kostya-sh - Good question. That approach would most certainly cause data races when running test methods in parallel. The main premise of this proposal is struct-level state, and for good reason.

@mdwhatcott

This comment has been minimized.

Copy link
Author

commented Aug 14, 2015

I forgot to mention that this proposal shouldn't require any changes to the code in the testing package. Maybe just an additional example of how to write struct-based tests in the documentation).

All changes that I foresee would happen in the go test command code.

@kostya-sh

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2015

@mdwhatcott, makes sense (about parallel run). Still I personally rather not have this functionality in go test tool (others may disagree):

  • the first version of the test from your proposal is actually shorter, more explicit and understandable without any explanations
  • the proposed functionality provides an alternative way to implement something that is already possible, it doesn't enable anything new
  • it requires more conventions to know about and remember in order to understand Go tests
@mdwhatcott

This comment has been minimized.

Copy link
Author

commented Aug 17, 2015

Regarding your first point: You are correct, the first version is shorter (it's also not the best example of how this convention could be used). Most of the time, common setup can be arranged into a table-driven test. Where there are more complex setup interactions, which may vary slightly for different test cases, the table-driven approach breaks down. In a struct-based scenario, common setup can be included in a Setup method, and then any additional setup can be performed by the individual test cases before performing the actions under test and asserting the correct results.

Regarding your second point: The beauty of the go test convention is that any test that is defined correctly is found automatically by the tool and executed. This proposal allows setup actions to be likewise discovered and executed for any related test cases without the need to explicitly call the setup/teardown functions from each test case. I suppose you could say that this doesn't introduce anything new, but it does introduce a much more reliable way to achieve common Setup/Teardown for related test cases.

Regarding the third point: Yes, this does introduce an additional convention. If someone wanted to make use of it in their tests, they would have to learn the convention. Any contributors would also have to learn the convention. Such is also true of every package in the standard library. But, this isn't a difficult convention to learn--a single example like the one I provided is enough to get the point across. Compared with learning the go concurrency model, or the mechanics of defer, panic, and recover this isn't too bad. It's also a convention that's already well-established in testing tools from other languages.

Again, no changes are necessary in the "testing" package as it already supports what I am proposing. (This is totally backward-compatible.) All that is required is to extend the current behavior of the go test command, a task I am confident I could implement correctly.

I'm happy to compose a formal design document if that would help clarify the proposal.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2015

No thanks.

Dealing with embedding means the go command (which drives the testing) would have to do its own type-checking of the program just to figure out where the tests are! That would also mean that human programmers might not be able to find the tests either. This seems too complex.

I think #12166 provides the needed flexibility in a much clearer way. Although that proposal calls itself table-driven test support, it's really more general than that.

@mdwhatcott

This comment has been minimized.

Copy link
Author

commented Oct 25, 2015

Thanks for your consideration--I really do appreciate it!

I respectfully disagree with your assertion that the approach is too complex, but I guess complexity is relative so we may be approaching this from different angles and with different expectations.

Having already implemented this approach, and having used it with my team for all new production code for several months now I'm really happy with how it works (we haven't experienced any difficulties like the ones you've mentioned).

https://github.com/smartystreets/gunit

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2015

I'm glad it works for you, but it still doesn't help programmatic generation of sub-tests, which comes up at least as often; in contrast #12166 does that. Please do look at golang.org/design/12166-subtests and see if there's anything with the approach you proposed that can't be done easily with that approach. We'd like to add just one way to define subtests.

I'm going to mark this declined because I think we want to rally around #12166 instead.

@rsc rsc closed this Oct 30, 2015

@mdwhatcott

This comment has been minimized.

Copy link
Author

commented Oct 30, 2015

Understood @rsc, thanks!

@golang golang locked and limited conversation to collaborators Nov 4, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.