Simple docker unit tests with network faking #1983
Conversation
Signed-off-by: pgillich <pgillich@gmail.com>
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
Just to be explicit, most of c/image is currently integration tested via https://github.com/containers/skopeo/tree/main/integration , which is a mandatory part of c/image CI. Are you seeing some real failures, or insufficient coverage there? (It’s quite possible that’s the case.)
Or is there some other specific thing you want to achieve with these tests?
That said, having fast unit tests that trigger failures immediately during development would certainly be useful; it’s just that with the Skopeo tests existing, we couldn’t find the time to do that.
Conceptually, I think unit test for the transports like docker://, and for the core copy logic in c/image/copy, are independent; a single test for a docker → docker-archive copy, exercising three separate packages, is really more of an integration test, and doing that at the top level with a Skopeo command seems good enough (given how thin a layer Skopeo itself is over c/image/copy).
Managing the fake HTTP responses manually seems to me to be a fairly large hassle, both to potentially extend, and to maintain, e.g. when the real-world responses change or we need to handle more kinds of them.
Compare old #254 for an alternative — record real-word interactions as files, not as Go code, and if something changes, just re-record them.
|
|
||
| // RegisterTestClientRoundTripper registers testClientRoundTripper, if set. | ||
| // Calls Transport.RegisterProtocol. | ||
| func (sys *SystemContext) RegisterTestClientRoundTripper(transport *http.Transport, schemes []string) *http.Transport { |
There was a problem hiding this comment.
I don’t think the internal unit test implementation should leak to the external API like this.
Instead, unit tests (assuming they don’t span multiple packages) can call the private newImageSource/newImageDestination with an internal-only extra parameter.
| Default: []signature.PolicyRequirement{ | ||
| signature.NewPRInsecureAcceptAnything(), | ||
| }, | ||
| Transports: map[string]signature.PolicyTransportScopes{ |
There was a problem hiding this comment.
With Default set, it should be possible to leave this empty.
|
I accept @mtrmac comments, his proposed way to make automatic tests is better and I close this PR. |
2 participants
These unit tests are faking network responses from docker repository. So coverage is increased without a real docker repository on the network.