-
Notifications
You must be signed in to change notification settings - Fork 28
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
faster Run for known types #166
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package quicktest_test | ||
|
||
import ( | ||
"testing" | ||
|
||
qt "github.com/frankban/quicktest" | ||
) | ||
|
||
func BenchmarkCNewAndRunWithCustomType(b *testing.B) { | ||
for i := 0; i < b.N; i++ { | ||
c := qt.New(customTForBenchmark{}) | ||
c.Run("test", func(c *qt.C) {}) | ||
} | ||
} | ||
|
||
func BenchmarkCRunWithCustomType(b *testing.B) { | ||
c := qt.New(customTForBenchmark{}) | ||
for i := 0; i < b.N; i++ { | ||
c.Run("test", func(c *qt.C) {}) | ||
} | ||
} | ||
|
||
type customTForBenchmark struct { | ||
testing.TB | ||
} | ||
|
||
func (customTForBenchmark) Run(name string, f func(testing.TB)) bool { | ||
return true | ||
} | ||
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,41 @@ | ||||||
// Licensed under the MIT license, see LICENSE file for details. | ||||||
|
||||||
//go:build go1.18 | ||||||
// +build go1.18 | ||||||
|
||||||
package quicktest | ||||||
|
||||||
import "testing" | ||||||
|
||||||
// fastRun implements c.Run for some known types. | ||||||
// It returns the result of calling c.Run and also reports | ||||||
// whether it was able to do so. | ||||||
func fastRun(c *C, name string, f func(c *C)) (bool, bool) { | ||||||
switch t := c.TB.(type) { | ||||||
case runner[*testing.T]: | ||||||
return fastRun1(c, name, f, t), true | ||||||
case runner[*testing.B]: | ||||||
return fastRun1(c, name, f, t), true | ||||||
case runner[*C]: | ||||||
return fastRun1(c, name, f, t), true | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be possible as a shortcut, but maybe a bit too clever. In fact this shortcut assumes that Another alternative which is less clever, but still shortcuts (avoids to create one
Suggested change
Note that this shortcut is already implemented in #165. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I thought about this, and even implemented it, but decided it wasn't a good idea. For one, it only saves one allocation, which is a fairly minor saving in this context. For another, it means that it's not consistent with the other types, which always allocate a new C. Making this optimisation changes observable behaviour and because I'm not convinced it's a worthwhile optimisation anyway, I'd be inclined to leave it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. One observable difference would be the format function that should be propagated from the parent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test proposed in #167. |
||||||
case runner[testing.TB]: | ||||||
// This case is here mostly for benchmarking, because | ||||||
// it's hard to create a working concrete instance of *testing.T | ||||||
// that isn't part of the outer tests. | ||||||
return fastRun1(c, name, f, t), true | ||||||
} | ||||||
return false, false | ||||||
} | ||||||
|
||||||
type runner[T any] interface { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very nice way of not exposing generics in the public API! |
||||||
Run(name string, f func(T)) bool | ||||||
} | ||||||
|
||||||
func fastRun1[T testing.TB](c *C, name string, f func(*C), t runner[T]) bool { | ||||||
return t.Run(name, func(t2 T) { | ||||||
c2 := New(t2) | ||||||
defer c2.Done() | ||||||
c2.SetFormat(c.getFormat()) | ||||||
f(c2) | ||||||
}) | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// Licensed under the MIT license, see LICENSE file for details. | ||
|
||
//go:build go1.18 | ||
// +build go1.18 | ||
|
||
package quicktest_test | ||
|
||
import ( | ||
"reflect" | ||
"testing" | ||
|
||
qt "github.com/frankban/quicktest" | ||
) | ||
|
||
type customT2[T testing.TB] struct { | ||
testing.TB | ||
} | ||
|
||
func (t *customT2[T]) Run(name string, f func(T)) bool { | ||
f(*new(T)) | ||
return true | ||
} | ||
|
||
func (t *customT2[T]) rtype() reflect.Type { | ||
return reflect.TypeOf((*T)(nil)).Elem() | ||
} | ||
|
||
type otherTB struct { | ||
testing.TB | ||
} | ||
|
||
func TestCRunCustomTypeWithNonMatchingRunSignature(t *testing.T) { | ||
// Note: this test runs only on >=go1.18 because there isn't any | ||
// code that specializes on this types that's enabled on versions before that. | ||
tests := []interface { | ||
testing.TB | ||
rtype() reflect.Type | ||
}{ | ||
&customT2[*testing.T]{}, | ||
&customT2[*testing.B]{}, | ||
&customT2[*qt.C]{}, | ||
&customT2[testing.TB]{}, | ||
&customT2[otherTB]{}, | ||
} | ||
for _, test := range tests { | ||
t.Run(test.rtype().String(), func(t *testing.T) { | ||
c := qt.New(test) | ||
called := 0 | ||
c.Run("test", func(c *qt.C) { | ||
called++ | ||
if test.rtype().Kind() != reflect.Interface && reflect.TypeOf(c.TB) != test.rtype() { | ||
t.Errorf("TB isn't expected type (want %v got %T)", test.rtype(), c.TB) | ||
} | ||
}) | ||
if got, want := called, 1; got != want { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also maybe s/called/callCount/, as called feels like a boolean. |
||
t.Errorf("subtest was called %d times, not once", called) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// Licensed under the MIT license, see LICENSE file for details. | ||
|
||
//go:build !go1.18 | ||
// +build !go1.18 | ||
|
||
package quicktest | ||
|
||
func fastRun(c *C, name string, f func(c *C)) (bool, bool) { | ||
return false, false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmarks could be moved to a separate PR as they would be useful for the other alternatives.