pkg/docker/config/testdata: Add credential-file fixtures #593

wking · 2019-03-01T07:35:54Z

We're primarily interested in maintaing compatibility with user's on-disk configurations, and using fixtures makes it harder to get that wrong.

While I'm touching these tests, also use t.Run and assert.Equal to make it easier to read both the source and any test failure messages.

rhatdan · 2019-03-01T08:57:48Z

Something is segfaulting.

wking · 2019-03-01T10:01:40Z

Something is segfaulting.

From Travis:

goroutine 5 [syscall]:
runtime.cgocall(0x8a4fa0, 0xc000097c60, 0x29)
	/usr/local/go/src/runtime/cgocall.go:128 +0x5e fp=0xc000097c28 sp=0xc000097bf0 pc=0x406e1e
github.com/containers/image/vendor/github.com/mtrmac/gpgme._Cfunc_gpgme_op_sign(0x16d2850, 0x16d55f0, 0x16d6640, 0x0, 0x0)
	_cgo_gotypes.go:936 +0x4d fp=0xc000097c60 sp=0xc000097c28 pc=0x6c148d
github.com/containers/image/vendor/github.com/mtrmac/gpgme.(*Context).Sign.func4(0x16d2850, 0x16d55f0, 0x16d6640, 0xc000000000, 0x0)
	/gopath/src/github.com/containers/image/vendor/github.com/mtrmac/gpgme/gpgme.go:481 +0xcc fp=0xc000097c98 sp=0xc000097c60 pc=0x6c7fcc
github.com/containers/image/vendor/github.com/mtrmac/gpgme.(*Context).Sign(0xc00041ef90, 0xc000097d48, 0x1, 0x1, 0xc00048e000, 0xc00048e080, 0x0, 0x120, 0x113)
	/gopath/src/github.com/containers/image/vendor/github.com/mtrmac/gpgme/gpgme.go:481 +0x109 fp=0xc000097ce8 sp=0xc000097c98 pc=0x6c4ff9
github.com/containers/image/signature.(*gpgmeSigningMechanism).Sign(0xc00048a000, 0xc0000bfe60, 0x113, 0x120, 0x9d0e63, 0x28, 0x0, 0xc00041d4f0, 0x10, 0xc00041d4d8, ...)
	/gopath/src/github.com/containers/image/signature/mechanism_gpgme.go:135 +0x178 fp=0xc000097d60 sp=0xc000097ce8 pc=0x865b68

But I'm having trouble figuring out how I could have broken anything over in that package with:

$ git show --oneline --stat
13eff85 pkg/docker/config/testdata: Add credential-file fixtures
 pkg/docker/config/config_test.go         | 314 ++++++++++---------------------
 pkg/docker/config/testdata/abnormal.json |  22 +++
 pkg/docker/config/testdata/example.json  |   7 +
 pkg/docker/config/testdata/full.json     |  25 +++
 pkg/docker/config/testdata/legacy.json   |   8 +
 5 files changed, 165 insertions(+), 211 deletions(-)

mtrmac · 2019-03-02T11:20:43Z

That crash is certainly unrelated (unless this happens to modify the memory layout to make it reproducible? That would be great); I’ve been trying to figure out the cause but I haven’t found anything so far.

wking · 2019-03-02T19:58:27Z

Closed/opened to kick Travis, and now we're green.

wking · 2019-03-05T06:43:14Z

Anything I can do to help this along? 👼

wking · 2019-03-06T20:23:13Z

Rebased onto master with 13eff85 -> 4520888 to resolve the "This branch is out-of-date with the base branch" warning. Still happy to help with review, if there's anything I can do...

wking · 2019-03-18T14:24:27Z

Bump :). This seems fairly straightforward and is green...

vrothberg · 2019-03-25T12:48:23Z

@wking, apologies for missing this PR. Would you mind rebasing? @mtrmac PTAL.

rhatdan · 2019-04-03T20:38:51Z

@wking ping ...

rhatdan · 2019-04-25T09:01:00Z

@wking Please rebase.

We're primarily interested in maintaing compatibility with user's on-disk configurations, and using fixtures makes it harder to get that wrong. While I'm touching these tests, also use t.Run and assert.Equal to make it easier to read both the source and any test failure messages. Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2019-04-25T19:32:31Z

Sorry for the delay. Rebased (no conflicts) with 4520888 -> 1c0fb51.

vrothberg

LGTM, thanks!

mtrmac · 2019-05-11T17:45:45Z

This introduces a flake (depending on in which order normalizedAuths are processed):

--- FAIL: TestGetAuth (0.01s)
    config_test.go:71: using temporary XDG_RUNTIME_DIR directory: "/var/folders/14/d3kqjd4d57q1bff3sph57ks80000gn/T/test_docker_client_get_auth799396144"
    config_test.go:87: using temporary home directory: "/var/folders/14/d3kqjd4d57q1bff3sph57ks80000gn/T/test_docker_client_get_auth026490575"
    --- FAIL: TestGetAuth/normalize_registry#01 (0.00s)
        Error Trace:    config_test.go:218
        Error:      	Not equal: 
                    	expected: "docker"
                    	received: "index"
        Error Trace:    config_test.go:219
        Error:      	Not equal: 
                    	expected: "io"
                    	received: "docker.io"
FAIL
FAIL	github.com/containers/image/pkg/docker/config	0.121s

While the commit says “add fixtures”, it seems that not a single affected test case has actually been just ported without modification, and there’s no rationale for those modifications. So, while moving to fixtures instead of autogenerating data, and the use of t.Run and assert.Equal, are, as such, certainly valuable, this is all, along with the test suite changes, conflated in a single commit, so it’s not trivially easy to preserve the useful parts without the parts that may need more discussion; I’m afraid I’m going to revert all of this for now.

wking · 2019-05-11T21:50:12Z

While the commit says “add fixtures”, it seems that not a single affected test case has actually been just ported without modification, and there’s no rationale for those modifications.

Motivation was consolidating the number of fixture files, vs. the previous tests which had many arbitrary, per-test-case data variations. I tried to preserve only the variation necessary to excercise the tested behavior. Not sure about the flake yet.

In 1c0fb51 (pkg/docker/config/testdata: Add credential-file fixtures, 2019-02-28, containers#593), I changed the test data for this test from: makeTestAuthConfig(testAuthConfigDataMap{ "docker.io": testAuthConfigData{"user", "pw"}, "localhost:5000": testAuthConfigData{"joe", "pass"}, }) to the full.json fixture which includes: { "auths": { "example.org": { "auth": "ZXhhbXBsZTpvcmc=" }, "index.docker.io": { "auth": "aW5kZXg6ZG9ja2VyLmlv" }, "docker.io": { "auth": "ZG9ja2VyOmlv" }, ... } } That lead ordering instability matching the normalized docker.io, with it occasionally matching the expected docker.io entry, and occasionally matching the index.docker.io entry. With this commit, I'm adjusting the hostname to be based on example.org to avoid such ambiguity in target matching while still excercising all the normalization logic that was being excercised before 1c0fb51. Signed-off-by: W. Trevor King <wking@tremily.us>

mtrmac · 2019-05-13T19:37:02Z

I don’t doubt the good intention of the motivation, and, to be clear, the flake has been clearly useful to show that the config handler should be improved; whether or not such ambiguous input can legitimately happen, handling even unlikely ambiguous input consistently over time would help reliability.

Still, it’s not at all trivial to determine that the old and new tests all have the same effect, and AFAICS the interpretation of this ambiguous input has been inconsistent this way in projectatomic/docker for a long time, so neither of these is clearly urgent; meanwhile we actively need need to get #625 and other mirror changes in before the next release, and these lower-priority changes with no clear user benefit slow that down.

I’ll let @vrothberg decide this, I’m clearly not unbiased.

mtrmac · 2019-05-13T20:13:33Z

I don’t doubt the good intention of the motivation

Also, using frozen files is clearly better than generating them at test time, risking that the generator code can be refactored along with the consumers, breaking consumption of old files without anyone noticing.

vrothberg · 2019-05-14T11:16:09Z

Let's move the discussion over to #626.

wking mentioned this pull request Mar 1, 2019

pkg/docker/config: Bring auth tests from docker/docker_client_test #589

Merged

wking closed this Mar 2, 2019

wking reopened this Mar 2, 2019

wking force-pushed the docker-config-test-fixtures branch from 13eff85 to 4520888 Compare March 6, 2019 20:22

wking force-pushed the docker-config-test-fixtures branch from 4520888 to 1c0fb51 Compare April 25, 2019 19:31

vrothberg reviewed Apr 26, 2019

View reviewed changes

vrothberg merged commit 54d6f62 into containers:master Apr 26, 2019

wking deleted the docker-config-test-fixtures branch April 26, 2019 13:53

mtrmac mentioned this pull request May 11, 2019

Revert "pkg/docker/config/testdata: Add credential-file fixtures" #624

Closed

wking mentioned this pull request May 11, 2019

pkg/docker/config/config_test: Stabilize "normalize registry" test #626

Merged

pkg/docker/config/testdata: Add credential-file fixtures #593

pkg/docker/config/testdata: Add credential-file fixtures #593

Uh oh!

Conversation

wking commented Mar 1, 2019

Uh oh!

rhatdan commented Mar 1, 2019

Uh oh!

wking commented Mar 1, 2019

Uh oh!

mtrmac commented Mar 2, 2019

Uh oh!

wking commented Mar 2, 2019

Uh oh!

wking commented Mar 5, 2019

Uh oh!

wking commented Mar 6, 2019

Uh oh!

wking commented Mar 18, 2019

Uh oh!

vrothberg commented Mar 25, 2019

Uh oh!

rhatdan commented Apr 3, 2019

Uh oh!

rhatdan commented Apr 25, 2019

Uh oh!

wking commented Apr 25, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented May 11, 2019

Uh oh!

mtrmac commented May 13, 2019

Uh oh!

mtrmac commented May 13, 2019

Uh oh!

vrothberg commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtrmac commented May 11, 2019 •

edited

Loading