Conversation
Awesome. I've run into issues with images not being cleaned up while working on push/pull tests. This will be a big improvement.
Agreed. |
There was a problem hiding this comment.
As this is implemented now, the name DockerPushPullSuite is misleading, because this isn't usable for pushes (no private registry). I think we should do one of the following:
- This should be renamed to something like
DockerHubPullSuite, and tests involving a private registry should be part of a different testsuite (possiblyDockerRegistrySuite, with changes to make it similar to this testsuite).
or
- This testsuite could provide a private registry, and replace
DockerRegistrySuite.
There was a problem hiding this comment.
That's absolutely right, I'll rename to DockerHubPullSuite, thanks!
0f36a2d to
3b61827
Compare
Introduce the `DockerHubPullSuite` that interacts with its own dedicated daemon, thus allowing to start from a clean environment and finely test against the impact of isolated push and pull operations. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>
3b61827 to
1a326a7
Compare
|
ping @vdemeester Who is also working on cleaning up |
There was a problem hiding this comment.
Probably just a nit (and because I think we don't use enough the power of check) — I'm writing this by memory so there might be a typo 😅 :
matches = len(digestExpr.FindAllStringSubmatch(out, -1))
c.Check(matches, check.Equals, 1, check.Commentf("expected exactly one image digest in the output, got %d", matches))
_, err := digest.ParseDigest(matches[0][1])
c.Check(err, check.IsNil, check.Commentf("invalid digest %q in output", matches[0][1]))Below you could do it with check.Matches, etc.. but I also feel like we should have our own Checker for the docker project (but that's another story 😉).
There was a problem hiding this comment.
I think you're right. @icecrime can try at least. Maybe it's not so easy as we think, but worth a try.
|
Test looks good and I'm 👍 of adding more test suites that are smaller and make sense 😉. Small nit with the use (or not) of On the
Agreed too. |
|
I'm not happy with this yet, give me a moment ;-) |
Add a `checker` package that adds some utility Checker implementation, the first one being `checker.Contains`, as well as brining all go-check provided Checker implementations in scope as a commodity. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>
Update and migrate existing tests to the `DockerHubPullSuite`. Most tests were preserved, but refactored and made more exhaustive. One test was deliberately removed (`TestPullVerified`) as it is unreliable and that the feature was obsoleted by content trust. Move all trust related tests to `docker_cli_pull_trusted_test.go`. Move tests depending on a local registry to `docker_cli_pull_local_test.go`. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>
1a326a7 to
f324f48
Compare
|
Updated with much better use of |
|
LGTM |
|
@icecrime this looks good 😻 |
|
LGTM |
|
something is going on w windows its unrelated, LGTM |
This is a first step into improving code coverage of our push/pull code.
DockerPushPullSuitethat interacts with its own dedicated daemon, thus allowing to start from a clean environment and finely test against the impact of isolated push and pull operations.DockerPushPullSuite. Most tests were preserved, but refactored and made more exhaustive. One test was deliberately removed (TestPullVerified) as it is unreliable and that the feature was obsoleted by content trust.docker_cli_pull_trusted_test.go.docker_cli_pull_local_test.go.After this PR gets in, I'll proceed in adding new test scenarios, and doing the same work on the push tests.
One thing I should mention (and feel free to file as an issue, or submit a PR) is that I'm not happy with the way
DockerPushPullSuiteis: it's too easy to incorrectly call todockerCmdand hit the wrong daemon. I believe the global integration tests daemon started as part ofhack/make/.integration-daemon-startshould be modeled by aDaemoninstance in the testing code, and that no function should execute commands without acting against a specific instance.