-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fixes some unit tests to be able to run them on windows #906
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| //+build linux | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dnephin it doesn't build at all on windows — there was a skip before 😓 |
||
|
|
||
| package image | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "io" | ||
| "io/ioutil" | ||
| "syscall" | ||
| "testing" | ||
|
|
||
| "github.com/docker/cli/internal/test" | ||
| "github.com/docker/docker/api/types" | ||
| "github.com/docker/docker/pkg/archive" | ||
| "github.com/gotestyourself/gotestyourself/fs" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "golang.org/x/net/context" | ||
| ) | ||
|
|
||
| func TestRunBuildResetsUidAndGidInContext(t *testing.T) { | ||
| dest := fs.NewDir(t, "test-build-context-dest") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should'nt we run this test on Darwin as well ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @simonferquel yes, this is also on my TODO (but I should open an issue to track the work on that) |
||
| defer dest.Remove() | ||
|
|
||
| fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) { | ||
| assert.NoError(t, archive.Untar(context, dest.Path(), nil)) | ||
|
|
||
| body := new(bytes.Buffer) | ||
| return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil | ||
| } | ||
| cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeImageBuild}) | ||
|
|
||
| dir := fs.NewDir(t, "test-build-context", | ||
| fs.WithFile("foo", "some content", fs.AsUser(65534, 65534)), | ||
| fs.WithFile("Dockerfile", ` | ||
| FROM alpine:3.6 | ||
| COPY foo bar / | ||
| `), | ||
| ) | ||
| defer dir.Remove() | ||
|
|
||
| options := newBuildOptions() | ||
| options.context = dir.Path() | ||
|
|
||
| err := runBuild(cli, options) | ||
| require.NoError(t, err) | ||
|
|
||
| files, err := ioutil.ReadDir(dest.Path()) | ||
| require.NoError(t, err) | ||
| for _, fileInfo := range files { | ||
| assert.Equal(t, uint32(0), fileInfo.Sys().(*syscall.Stat_t).Uid) | ||
| assert.Equal(t, uint32(0), fileInfo.Sys().(*syscall.Stat_t).Gid) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "io/ioutil" | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| "github.com/docker/cli/internal/test" | ||
|
|
@@ -14,7 +15,10 @@ import ( | |
| ) | ||
|
|
||
| func TestCreateErrors(t *testing.T) { | ||
|
|
||
| noSuchFile := "no such file or directory" | ||
| if runtime.GOOS == "windows" { | ||
| noSuchFile = "The system cannot find the file specified." | ||
| } | ||
| testCases := []struct { | ||
| args []string | ||
| expectedError string | ||
|
|
@@ -29,7 +33,7 @@ func TestCreateErrors(t *testing.T) { | |
| }, | ||
| { | ||
| args: []string{"plugin-foo", "nonexistent_context_dir"}, | ||
| expectedError: "no such file or directory", | ||
| expectedError: noSuchFile, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I recall some shenanigans like https://github.com/Microsoft/opengcs/blob/8cfbb6d8ae8203a80464ca6e2bd39102d6fd9deb/service/gcsutils/remotefs/utils.go#L93-L107 and https://github.com/Microsoft/opengcs/blob/8cfbb6d8ae8203a80464ca6e2bd39102d6fd9deb/service/gcsutils/remotefs/utils.go#L36-L50
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! you're right, was thinking about Linux CLI calling a Windows daemon (in which errors don't match), but that's not the case here |
||
| }, | ||
| } | ||
|
|
||
|
|
@@ -61,7 +65,11 @@ func TestCreateErrorOnContextDirWithoutConfig(t *testing.T) { | |
| cmd := newCreateCommand(cli) | ||
| cmd.SetArgs([]string{"plugin-foo", tmpDir.Path()}) | ||
| cmd.SetOutput(ioutil.Discard) | ||
| testutil.ErrorContains(t, cmd.Execute(), "config.json: no such file or directory") | ||
| expectedErr := "config.json: no such file or directory" | ||
| if runtime.GOOS == "windows" { | ||
| expectedErr = "config.json: The system cannot find the file specified." | ||
| } | ||
| testutil.ErrorContains(t, cmd.Execute(), expectedErr) | ||
| } | ||
|
|
||
| func TestCreateErrorOnInvalidConfig(t *testing.T) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ func TestValidateExternalNetworks(t *testing.T) { | |
| expectedMsg: "Unexpected", | ||
| }, | ||
| { | ||
| // FIXME(vdemeester) that doesn't work under windows, the check needs to be smarter | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tests fails under Windows because of : 7f53c99#r27815425
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the info endpoint could be queried instead of making the assumption that daemon os/platform == client os/platform
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. definitely, but probably need to be "skipped" for now, and follow-up on a fix PR for that problem only 👼 |
||
| inspectError: errors.New("host net does not exist on swarm classic"), | ||
| network: "host", | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,13 @@ import ( | |
| "io/ioutil" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| "github.com/docker/cli/cli/config" | ||
| "github.com/docker/cli/internal/test" | ||
| "github.com/docker/cli/internal/test/testutil" | ||
| "github.com/gotestyourself/gotestyourself/skip" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/theupdateframework/notary" | ||
| "github.com/theupdateframework/notary/passphrase" | ||
|
|
@@ -20,6 +22,10 @@ import ( | |
| ) | ||
|
|
||
| func TestTrustKeyLoadErrors(t *testing.T) { | ||
| noSuchFile := "stat iamnotakey: no such file or directory" | ||
| if runtime.GOOS == "windows" { | ||
| noSuchFile = "CreateFile iamnotakey: The system cannot find the file specified." | ||
| } | ||
| testCases := []struct { | ||
| name string | ||
| args []string | ||
|
|
@@ -40,7 +46,7 @@ func TestTrustKeyLoadErrors(t *testing.T) { | |
| { | ||
| name: "not-a-key", | ||
| args: []string{"iamnotakey"}, | ||
| expectedError: "refusing to load key from iamnotakey: stat iamnotakey: no such file or directory", | ||
| expectedError: "refusing to load key from iamnotakey: " + noSuchFile, | ||
| expectedOutput: "Loading key from \"iamnotakey\"...\n", | ||
| }, | ||
| { | ||
|
|
@@ -109,6 +115,7 @@ var testKeys = map[string][]byte{ | |
| } | ||
|
|
||
| func TestLoadKeyFromPath(t *testing.T) { | ||
| skip.If(t, func() bool { return runtime.GOOS == "windows" }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of the Maybe add a reason why as well? |
||
| for keyID, keyBytes := range testKeys { | ||
| t.Run(fmt.Sprintf("load-key-id-%s-from-path", keyID), func(t *testing.T) { | ||
| testLoadKeyFromPath(t, keyID, keyBytes) | ||
|
|
@@ -163,6 +170,7 @@ func testLoadKeyFromPath(t *testing.T, privKeyID string, privKeyFixture []byte) | |
| } | ||
|
|
||
| func TestLoadKeyTooPermissive(t *testing.T) { | ||
| skip.If(t, func() bool { return runtime.GOOS == "windows" }) | ||
| for keyID, keyBytes := range testKeys { | ||
| t.Run(fmt.Sprintf("load-key-id-%s-too-permissive", keyID), func(t *testing.T) { | ||
| testLoadKeyTooPermissive(t, keyBytes) | ||
|
|
@@ -219,6 +227,7 @@ H3nzy2O6Q/ct4BjOBKa+WCdRtPo78bA+C/7t81ADQO8Jqaj59W50rwoqDQ== | |
| -----END PUBLIC KEY-----`) | ||
|
|
||
| func TestLoadPubKeyFailure(t *testing.T) { | ||
| skip.If(t, func() bool { return runtime.GOOS == "windows" }) | ||
| pubKeyDir, err := ioutil.TempDir("", "key-load-test-pubkey-") | ||
| assert.NoError(t, err) | ||
| defer os.RemoveAll(pubKeyDir) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arf
Is this dependent on system configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure 😓 cc @simonferquel @johnstep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems. the good thing is that the timezone is in the string here, and that the format is in a standard parsable from the time package. Comparison should be done after date/time parsing instead that on strings would be better though (and would not require windows specific code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're testing a formatter, which is responsible for formatting output that is seen by a user, I think we should be comparing the string, not a
time.Time. The format is relevant here.