-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
testutil: Moving all e2e utils to separate package, added TB union. #2077
Conversation
14a8415
to
da4090d
Compare
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.
lgtm. I have a couple of ignorable comments.
cmd/thanos/main_test.go
Outdated
@@ -11,6 +11,7 @@ import ( | |||
"path/filepath" | |||
|
|||
"github.com/thanos-io/thanos/pkg/block/metadata" | |||
"github.com/thanos-io/thanos/pkg/testutil/e2eutil" |
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.
nit: Shall we group the imports? Does it even matter for the tests?
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.
we should but we don't have tools for that so it's tedious and I am lazy (:
@jojohappy if you were asking if still valid: #984
pkg/testutil/testorbench.go
Outdated
} | ||
|
||
func (tb *UnionTB) IsBenchmark() bool { | ||
if _, ok := tb.TB.(*testing.B); ok { |
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.
I'm totally NOT sure if this makes it easier to read or not, but the following could also be used
if _, ok := tb.TB.(*testing.B); ok { | |
_, ok := tb.TB.(*testing.B) | |
return ok |
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.
Makes sense 👍
pkg/testutil/testorbench.go
Outdated
func (tb *UnionTB) ResetTimer() { | ||
if b, ok := tb.TB.(*testing.B); ok { | ||
b.ResetTimer() | ||
return |
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.
very minor nit: not needed return :P
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.
LGTM! Only some small nits from @kakkoyun and @GiedriusS .
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com