Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

specs-go/round_trip_test: Add round-trip testing for the config #759

Closed
wants to merge 4 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Apr 6, 2017

To make sure the Go structs can accurately transport known-good configurations.

Also use a pointer for Process.ConsoleSize to avoid:

$ go test ./round_trip_test.go
--- FAIL: TestConfigRoundTrip (0.00s)
    --- FAIL: TestConfigRoundTrip/config_1 (0.00s)
        round_trip_test.go:32: failed to round-trip:
                {"ociVersion":"1.0.0","platform":{"os":"linux","arch":"amd64"},"process":{"consoleSize":{"height":0,"width":0},"user":{"uid":1,"gid":1},"args":["sh"],"cwd":"/"},"root":{"path":"rootfs"}}
FAIL
FAIL    command-line-arguments  0.003s

Background on this in golang/go#11939.

@wking
Copy link
Contributor Author

wking commented May 9, 2017

The less contentious parts of this have been spun off into #791 and #792. Probably look at those first.

I'll rebase this one on top once they are dealt with, and we can decide if there's anything else in here worth keeping or give reasons for rejecting any remaining changes.

@tianon
Copy link
Member

tianon commented May 11, 2017

I think this is ready for the mentioned rebase? I'm personally pretty indifferent on the change either way, and don't think it really warrants holding up 1.0 (seems like a good way for us to verify our work is self-consistent, but not a strict requirement).

wking added 4 commits May 10, 2017 23:51
To make sure the Go structs can accurately transport known-good
configurations.

Use Docker's canonical JSON implementation so we don't have to worry
about false-positives due to JSON whitespace, key order, etc.

Signed-off-by: W. Trevor King <wking@tremily.us>
And run them from Travis.

Also replace some old Travis test commands with a single call to 'make
test'.  Now we die on the first error (e.g. we don't run the Go tests
if lint fails), but I don't think that's a big problem, and this way
we don't have to maintain two parallel lists of test targets.

Signed-off-by: W. Trevor King <wking@tremily.us>
process.user.username is not strictly required yet, but it is intended
to be [1], and it doesn't seem worth making Process.User a pointer in
the meantime.

[1]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
Avoiding:

  $ go test ./specs-go
  --- FAIL: TestConfigRoundTrip (0.00s)
      --- FAIL: TestConfigRoundTrip/config_2 (0.00s)
          round_trip_test.go:35: failed to round-trip:
                  {"ociVersion":"1.0.0","platform":{"os":"windows","arch":"amd64"},"process":{"user":{"uid":0,"gid":0,"username":"containeradministrator"},"args":["sh"],"cwd":"C:\\"},"root":{"path":"rootfs"}}
  FAIL
  FAIL    github.com/opencontainers/runtime-spec/specs-go 0.003s

We can't just add omitempty (without also making them pointers),
because 0 is a meaningful value for both properties.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the round-trip-config-tests branch from 750562b to 05415d4 Compare May 11, 2017 06:53
@wking
Copy link
Contributor Author

wking commented May 11, 2017 via email

@mrunalp mrunalp closed this May 12, 2017
@wking
Copy link
Contributor Author

wking commented May 12, 2017

@mrunalp, I think you were going to add a comment (or file a style.md PR?) about the pointer pattern so I don't mis-paraphrase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants