Add a Run helper method #510
Conversation
| @@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions { | |||
| return suite.Assertions | |||
| } | |||
|
|
|||
| // Run is a helper method to do a simple subtest. It sets `T()` to the | |||
| // `*testing.T` of the subtest. | |||
| func (suite *Suite) Run(name string, f func()) { | |||
There was a problem hiding this comment.
@JelteF could the name be confusing considering we already have suite.Run?
|
|
||
| defer func() { | ||
| suite.SetT(oldT) | ||
| }() |
There was a problem hiding this comment.
Could this result in race conditions if somebody called suite.T().Parallel() within the subtest?
| suite.SetT(oldT) | ||
| }() | ||
|
|
||
| f() |
There was a problem hiding this comment.
Rather than receiving an empty func(), maybe we should receive func(*Suite) and send a test suite with the new *testingT?
Would that prevent race conditions?
|
The name might be a bit confusing, but I think it's quite useful for
clarity to use the same name as the normal testing library uses.
Regarding race conditions I was convinced the suite based tests didn't
support parallel tests anyway, because of their setup and teardown
mechanics. If so I don't think it's a problem at the moment to not support
T().Parallel() here either. (I'm not at my computer right now, so I can't
look up the issue describing the lack of support, but I think it's quite
easy to find on Google)
…On Sat, Dec 30, 2017, 14:36 Ernesto Jiménez ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I think this might have some race conditions.
It would be good to have some accompanying tests too.
------------------------------
In suite/suite.go
<#510 (comment)>:
> @@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions {
return suite.Assertions
}
+// Run is a helper method to do a simple subtest. It sets `T()` to the
+// `*testing.T` of the subtest.
+func (suite *Suite) Run(name string, f func()) {
@JelteF <https://github.com/jeltef> could the name be confusing
considering we already have suite.Run?
------------------------------
In suite/suite.go
<#510 (comment)>:
> @@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions {
return suite.Assertions
}
+// Run is a helper method to do a simple subtest. It sets `T()` to the
+// `*testing.T` of the subtest.
+func (suite *Suite) Run(name string, f func()) {
+ oldT := suite.T()
+ suite.T().Run(name, func(t *testing.T) {
+ suite.SetT(t)
+
+ defer func() {
+ suite.SetT(oldT)
+ }()
Could this result in race conditions if somebody called
suite.T().Parallel() within the subtest?
------------------------------
In suite/suite.go
<#510 (comment)>:
> @@ -55,6 +55,21 @@ func (suite *Suite) Assert() *assert.Assertions {
return suite.Assertions
}
+// Run is a helper method to do a simple subtest. It sets `T()` to the
+// `*testing.T` of the subtest.
+func (suite *Suite) Run(name string, f func()) {
+ oldT := suite.T()
+ suite.T().Run(name, func(t *testing.T) {
+ suite.SetT(t)
+
+ defer func() {
+ suite.SetT(oldT)
+ }()
+
+ f()
Rather than receiving an empty func(), maybe we should receive
func(*Suite) and send a test suite with the new *testingT?
Would that prevent race conditions?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#510 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG8JltFMeZM6xKwv4LTC1iQfrg3lPMcks5tFjxBgaJpZM4P2X5m>
.
|
|
@JelteF parallel suite tests are currently unsupported because, but I'm unsure what the situation would be with subtests and whether we would be able to support parallel suite tests in the future. The following might help with that: func (s *Suite) RunSubtest(name string, fn fun(*Suite))Where the suite sent is a different one with the new What do you think? |
|
First off, did you mean to close the PR? Regarding the proposed change, I don't think I really like it. Mainly because I wouldn't have access to the helper methods I have on my custom Suite type. Renaming it to RunSubtest is fine by me though if you think that causes less confusion. |
|
Hey, sorry, clicked the wrong button. You could implement this: // RunSubtest receives the name of the subtest and a function taking a single argument of a struct that embeds `Suite`. The subtest will be called with a new instance of the type provided in the argument with the subtest’s `*testing.T`
func (s *Suite) RunSubtest(name string, fn interface{})Then, using reflection you would set things up. We do something similar in the Even if running tests in parallel is currently broken, it would be good to fix that bug rather than relying on the bug. |
|
Would it be okay if I add a |
|
PR is not actual more? |
|
Apparently not. |
I love table tests, especially when using Run. And this adds that functionality to the
suitelibrary.