Using t.Parallel() with Convey dies #70
Comments
ghost
assigned 
|
@augustoroman I'm having trouble replicating that error message. Can you paste the code you used in the bowling_game_test.go file? It looks like you might have added a I was able to add a single call to |
|
Please see https://github.com/augustoroman/goconvey/commit/6330da9ee523ae253aaa80e22b563e98eae007ca I verified that this occurs for me with go1.2rc2 and go1.2rc3, but not go1.1.2. Might be a regression. |
|
@augustoroman Ok, that's just like what I tried. I'm on go1.1.2 right now and there is an open issue related to getting go 1.2 working. Right now I'm working on some internal refactoring on the server but the Go 1.2 stuff will be addressed quickly. Stay tuned... |
|
It's not related to the server, but rather to core functionality in GoConvey. The problem seems to be that there's a single global runner variable that is not safe for concurrent access. Running the tests with the race detector exposed the problem: In I'm not sure how extensive your refactoring is. The globals should be removed if possible. |
|
Thanks for your detective work here @augustoroman. You're absolutely right about the globals and I've known they would pose a problem where concurrency is concerned. We won't be able to remove them because this code is imported as a library and is subordinate to |
|
Not quite sure how to go about this yet so we are deferring this issue. It will be referenced on the wiki for future discussion. |
|
Ok, I've got some ideas on how to solve this. It's really been bugging me that we don't support this feature. Making it work would involve a few trade-offs:
Here's what the code would look like: package examples
import (
. "github.com/smartystreets/goconvey/convey"
"testing"
)
func TestSpec(t *testing.T) {
Convey("Subject: Integer incrementation and decrementation", t, func(so Assert) {
var x int
Convey("Given a starting integer value", func() {
x = 42
Convey("When incremented", func() {
x++
Convey("The value should be greater by one", func() {
so(x, ShouldEqual, 43)
})
Convey("The value should NOT be what it used to be", func() {
so(x, ShouldNotEqual, 42)
})
})
Convey("When decremented", func() {
x--
Convey("The value should be lesser by one", func() {
so(x, ShouldEqual, 41)
})
Convey("The value should NOT be what it used to be", func() {
so(x, ShouldNotEqual, 42)
})
})
Reset(func() {
x = 0
})
})
})
}Notice that the top-level Thoughts? |
|
As I've mused about this alternate syntax I'm realizing that there are some intricacies like (SkipConvey, SkipSo, Reset, etc...) that will have to be worked through as well. All of those public methods are leveraging the package-level resources which I will be trying to get rid of. |
|
The change to the DSL is minor, albeit more complex/more to remember. (Even though we have a code generator, it's not used all the time.) Is it worth it? |
|
It's true that I've gone back and forth on this. Here are the current reasons for moving forward with the proposed change:
Reasons against the change:
|
|
Alternative DSL that makes skipping and resets easy (addresses item number 3 in my last comment): Convey("Stuff", t, func(c C) {
c.Convey("More stuff", func() {
c.So(1, ShouldEqual, 1)
})
c.SkipConvey("Skipped stuff", func() {
c.So(1, ShouldEqual, 2)
})
c.Reset(func() {
// stuff
})
})So this introduces a new struct |
|
Here's another version that's arguably cleaner: Convey("Stuff", t, func(c Context, so Assert) {
Convey("More stuff", c, func() {
so(1, ShouldEqual, 1)
})
SkipConvey("Skipped stuff", c, func() {
so(1, ShouldEqual, 2)
})
Reset(c, func() {
// stuff
})
})Benefits:
|
|
Ok, I'm closing this issue to start another regarding the changes we are considering with the DSL. In the event that a change is implemented it will include a fix and a reference for this issue. |
|
@augustoroman @mholt @joliver - I've fixed it! And it didn't require any changes to the DSL. I'm still hacking on things but you are welcome to check out the "issue70" branch to see the fix. The package-level state still exists but instead of one runner and one reporter there are now runners and reporters for each test function so there's no competing for resources (where was my brain 3 months ago?). I modified the example tests to use |
|
Awesome!! I tried it out and it was working really well, and I was about to call this done when I hit this: |
|
That doesn't look too bad; not anything a channel can't fix, I'd imagine? We just have to protect that map. |
|
Yup, not hard to fix. I've implemented a simple |
For example, modify the tests in examples to all use t.Parallel(), then run:
The following panic comes up:
The text was updated successfully, but these errors were encountered: