-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
zaptest: Expand TestingT interface #565
Conversation
Codecov Report
@@ Coverage Diff @@
## abg/test-logger #565 +/- ##
================================================
Coverage 97.48% 97.48%
================================================
Files 40 40
Lines 2026 2026
================================================
Hits 1975 1975
Misses 43 43
Partials 8 8 Continue to review full report at Codecov.
|
@@ -29,6 +29,15 @@ type TestingT interface { | |||
// Logs the given message and marks the test as failed. | |||
Errorf(string, ...interface{}) | |||
|
|||
// Marks the test as failed. | |||
Fail() |
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.
do we need Fail
when we already have Logf
and Errorf
?
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.
Fail marks the test as having failed without logging a message.
Errorf("")
will still emit an empty string.
We don't necessarily need this but I can see its value if you want to log a
multi-line complex message before you mark the test as failed.
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 OK either way, if we have use-cases where we want to log a message separately to using Errorf
, then we can keep it. If we haven't done this in our other projects, then perhaps we don't need it.
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.
How we mark tests as failed in normal tests in other projects isn't
that important here. This interface controls which methods are
available to our testing utility. If we decide to add more features to
the test logger in the future, it'll be difficult to request more of
the methods from testing.TB
.
The intent here is to get the minimal standalone functionality of
testing.TB. I wouldn't mind dropping Errorf in favor of just Logf and
Fail but we want Errorf for testify compatibility.
FWIW, we are using Fail in the logger returned by zaptest.NewLogger:
We use it when the SyncWriter needs to mark the test as failed if
anything gets written to ErrorOutput. (#566)
This expands the TestingT interface to a much larger subset of `testing.TB`. The following were left out: - `Error` and `Log` were left out in favor of `Errorf` and `Logf` - `Fatal*` methods were left out in favor of `Errorf` followed by `FailNow` - `Skip*` methods were left out because our test logger shouldn't be skipping tests - `Helper` was left out because not all supported verisons of Go have that
820a61b
to
cdd3f0b
Compare
Ping :) |
This expands the TestingT interface to a much larger subset of
testing.TB
.The following were left out:
Error
andLog
were left out in favor ofErrorf
andLogf
Fatal*
methods were left out in favor ofErrorf
followed byFailNow
Skip*
methods were left out because our test logger shouldn't beskipping tests
Helper
was left out because not all supported verisons of Go havethat
I'm putting this in a separate PR so that we can discuss every included/omitted
method outside #518.
CC @akshayjshah @prashantv