zaptest/NewLogger: fail test on internal errors #566
Conversation
Codecov Report
@@ Coverage Diff @@
## abg/expand-testingt #566 +/- ##
=======================================================
+ Coverage 97.48% 97.49% +0.01%
=======================================================
Files 40 40
Lines 2026 2035 +9
=======================================================
+ Hits 1975 1984 +9
Misses 43 43
Partials 8 8
Continue to review full report at Codecov.
|
zaptest/logger_test.go
Outdated
| assert.Equal(t.TB, msgs, t.Messages, "logged messages did not match") | ||
| } | ||
|
|
||
| func (t *testLogSpy) AssertFailed(v bool) { |
There was a problem hiding this comment.
Can we make this an unexported method and have methods AssertFailed() and AssertPassed() rather than a boolean argument?
| // testLogSpy is a testing.TB that captures logged messages. | ||
| type testLogSpy struct { | ||
| testing.TB | ||
|
|
||
| mu sync.Mutex | ||
| mu sync.RWMutex |
There was a problem hiding this comment.
optional/can follow up:
this spy is only used internally, and I don't think any of the tests are concurrent, so we might be able to just remove the mutex.
There was a problem hiding this comment.
The spy is thread safe only because testing.TB is supposed to be thread-
safe, but yeah, I'll drop it.
abhinav
force-pushed
the
abg/test-error-output
branch
from
2339e4e to
e1812df
Compare
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
Zap's errorOutput is used exclusively to log internal errors. If this ever gets used in a test, something catastrophic happened and we should fail the test. This changes the logger returned by zaptest.NewLogger to implement this behavior by passing a `WriteSyncer` to `zap.ErrorOutput` that marks the test as failed upon being used. To test this, we set up a test logger and replace its core with one that will always fail to write.
abhinav
force-pushed
the
abg/expand-testingt
branch
from
820a61b to
cdd3f0b
Compare
abhinav
force-pushed
the
abg/test-error-output
branch
from
e1812df to
2b3338c
Compare
Zap's errorOutput is used exclusively to log internal errors. If this
ever gets used in a test, something catastrophic happened and we should
fail the test.
This changes the logger returned by zaptest.NewLogger to implement this
behavior by passing a
WriteSyncertozap.ErrorOutputthat marks thetest as failed upon being used.
To test this, we set up a test logger and replace its core with one that
will always fail to write.