No internal package use in tests#1175
Draft
TBBle wants to merge 3 commits intomicrosoft:mainfrom
Draft
Conversation
a54b0e5 to
fbffeaa
Compare
An idea mentioned in-passing in microsoft#1073, this would ensure that tests can be looked at as usage examples, which use of internal APIs prevents. vendor/ and internal/ are excluded, the former because we only care about direct dependencies, and the latter so that if tests are done that depend on an unpublished API, it's possible _and_ explicit. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Some alternative approaches: * Move the relevant tests to tests/internal to make it clear it's testing internal stuff. * Restructure the four tests to use a compiled shimdiag.exe or host-provided hcsdiag.exe instead, assuming the latter is equivalent. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
fbffeaa to
df1b28a
Compare
Contributor
Author
|
If I or someone else ever come back to this, I saw a neat trick in the Go source itself, of having an |
This file contains hidden or 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
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.
Based on a comment I made earlier in #1073, I had a look at it would take to enforce a "No imports from
internalin tests" policy.A couple of easy, self-contained packages to make public fell out, but then the next thing I wanted to make public was
internal/cmd, and it exposes a couple of types in its APIs frominternal/cowandinternal/uvm, and the dependency graph explodes at that point, even before I looked at test/functional/wcow_test.go which pulls 10 different packages frominternalplushcsshimitself, probably because ofhcsshim/test/functional/wcow_test.go
Lines 29 to 36 in 1f8211a
which leaps-out as a super-code smell if it's testing old and new interfaces, and the entire "new" interface is not public.
So I'm posting this for direction/consideration/ideation:
pkg/for publishingociwclayerwas a mistake, but I guess it does send the signal that this is intended to be a more-stable API, within the constraints of the current v0.x version. (It also sends the signal that I was using last decade's best-practice due to learning Go from old tutorials). And Export hcsshim annotations into its own package #1201 has doubled down on this, so I guess this is where we're going now.internal/hcs/schemashould probably not be internal, or needs more wrapping functions. #1073, I'm not sure if we want to go wide on publishing modules, go narrow using a lot of type aliases, or even consider a bunch of helper functions to expose/wrap internal APIs.internal/hcs/schemashould probably not be internal, or needs more wrapping functions. #1073 and How is read-write layer mounting done on the host with computestorage? #1076, but are not blocked by this work particularly.