Break dependency between zap and zaptest #523
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akshayjshah
force-pushed
the
ajs-testing
branch
from
9e991ea
to
2635460
Compare
akshayjshah
changed the title
added 4 commits
October 31, 2017 10:07
In order to add more useful functionality to the `zaptest` package, we'll need to reference the top-level `zap` package. To do that, we need to break some import cycles. As a first step, copy the current testing utilities to an internal package.
Migrate all tests in the main `zap` package to use the internal `ztest` helpers. This lets the `zaptest` package take dependencies on `zap`.
Migrate the benchmarks package to use the internal `ztest` helpers instead of the exported `zaptest` package.
Use the `ztest` internal helper utilities in `zapcore`, which lets the `zaptest` package take dependencies on `zapcore`.
akshayjshah
force-pushed
the
ajs-testing
branch
from
2635460
to
a142dae
Compare
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
==========================================
+ Coverage 97.03% 97.06% +0.02%
==========================================
Files 37 39 +2
Lines 2257 2279 +22
==========================================
+ Hits 2190 2212 +22
Misses 59 59
Partials 8 8
Continue to review full report at Codecov.
|
prashantv
approved these changes
Oct 31, 2017
|
|
||
| var _timeoutScale = 1.0 | ||
| "go.uber.org/zap/internal/ztest" | ||
| ) | ||
|
|
||
| // Timeout scales the provided duration by $TEST_TIMEOUT_SCALE. |
There was a problem hiding this comment.
can you mark this as deprecated? I don't think these functions are intended for end-users right? This should purely be for internal testing
|
|
||
| // Timeout scales the provided duration by $TEST_TIMEOUT_SCALE. | ||
| func Timeout(base time.Duration) time.Duration { | ||
| return time.Duration(float64(base) * _timeoutScale) | ||
| return ztest.Timeout(base) | ||
| } | ||
|
|
||
| // Sleep scales the sleep duration by $TEST_TIMEOUT_SCALE. |
There was a problem hiding this comment.
same as above, should this be deprecated?
zaptest/writer_go_19.go
Outdated
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
||
| // +build go1.9 |
There was a problem hiding this comment.
optional: should we call out why we need 1.9 (e.g., that we're using type aliases?)
There was a problem hiding this comment.
Sure, sounds good!
|
|
||
| func readCode(t testing.TB, fname string) []string { | ||
| f, err := os.Open(fname) | ||
| require.NoError(t, err, "Failed to read %s.", fname) |
There was a problem hiding this comment.
defer f.Close() after the require.NoError
There was a problem hiding this comment.
🤦♂️ Good catch.
Mitigate the distasteful copy-paste of `zaptest` by providing a path forward using Go 1.9 aliases. For Go 1.9 users, `zaptest` is now nothing more than a few type aliases; this cleans up the GoDoc output nicely, since it removes the documentation for all the methods on the testing spies while remaining fully backward-compatible. For Go 1.8 and earlier, we keep the original code but add a test to make sure that it exactly matches the version in `internal/ztest`. With this change, we're free to introduce functionality into `zaptest` that relies on `zapcore` and `zap` itself. In particular, we can return `*zap.Logger` from constructors.
akshayjshah
force-pushed
the
ajs-testing
branch
from
a142dae
to
49f1c0e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to introduce new testing helpers into the
zaptestpackage, where users will expect to find them, we need to break the dependency betweenzapandzaptest. (Unfortunately, we introducedzaptestbefore Go supported internal packages and squatted on the best user-facing name.)This PR can be reviewed commit-by-commit if necessary. It makes a wordy, but logically simple series of changes: we copy the
zaptestpackage tointernal/ztest, change the rest ofzapto use the new internal helpers, and finally convertzaptestto a shim around the internal helpers. In Go 1.9, we use type aliases to also clean up the documentation (aliases don't have their methods documented); for older Go versions, we leave a copy of the original code and a test to make sure that the two versions don't drift out of sync.This should make #518 much nicer. Since it begins using type aliases, it depends on #524.