-
Notifications
You must be signed in to change notification settings - Fork 78
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
testr: merge testLogger and testLoggerInterface #148
Conversation
Signed-off-by: Timon Wong <timon86.wang@gmail.com>
@@ -116,7 +116,7 @@ type testlogger struct { | |||
} | |||
|
|||
func (l testlogger) GetUnderlying() *testing.T { | |||
return l.t.(*testing.T) | |||
return l.t.(*testing.T) //nolint:forcetypeassert |
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.
That something like this would be needed was the main reason for having two separate implementations.
But I think it's okay and worth the reduction in code.
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 don't get it - if we can make this assumption, then what value is the interface-based version?
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.
At the API level, both versions are type-safe and only deal with testing.T
resp. TestingT
. The implementation of testlogger
then reuses the interface-based version and therefore only has a l.t
of type TestingT
here although all that ever gets stored here is the testing.T
pointer. Therefore the typecast is necessary and safe.
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 could call NewWithInterface()
with something that ISN'T a testing.T
, right? Then this will panic. What am I missing?
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.
This was added so that we could pass a testing.B
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.
NewWithInterface()
returns a testloggerInterface
and thus doesn't have this testlogger.GetUnderlying
.
Only New(t *testing.T)
and NewWithOptions(t *testing.T, opts Options)
return a testlogger
instance.
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 agree that this is non-obvious. Perhaps it is better to keep the two implementations completely separate, despite the code duplication? Or add more code comments?
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.
Ahhhhhh I see. Yeah, a comment here like:
// This method is defined on testlogger, so the only type this could possibly be is testing.T
@@ -116,36 +112,11 @@ func logError(t TestingT, formatError func(error, string, []interface{}) (string | |||
} | |||
|
|||
type testlogger struct { |
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.
As per the below discussion, we should comment here that this exists to wrap and modify the method-set of testloggerInterface, in particular it changes the GetUnderlying() method
@@ -116,7 +116,7 @@ type testlogger struct { | |||
} | |||
|
|||
func (l testlogger) GetUnderlying() *testing.T { | |||
return l.t.(*testing.T) | |||
return l.t.(*testing.T) //nolint:forcetypeassert |
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.
Ahhhhhh I see. Yeah, a comment here like:
// This method is defined on testlogger, so the only type this could possibly be is testing.T
Ping - any desire to finish this? |
Hi @timonwong I'd like to maybe tag a release in the near future, and I'd like to see this in there. If you don't plan to finish it, do you object if I take it over? |
No description provided.