Fixes some unit tests to be able to run them on windows by vdemeester · Pull Request #906 · docker/cli

vdemeester · 2018-02-27T15:59:09Z

Some of them are skipped for now (because the feature is not supported
or needs more work), some of them are fixed.

There is still some errors, but it's a start 👼 🙏

Signed-off-by: Vincent Demeester [email protected]

vdemeester · 2018-02-27T16:00:04Z

cli/command/image/build_linux_test.go

+)
+
+func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	skip.IfCondition(t, runtime.GOOS == "windows", "uid and gid not relevant on windows")


This check is actually not relevant as assert.Equal(t, uint32(0), fileInfo.Sys().(*syscall.Stat_t).Uid) doesn't compile under windows… I should remove that skip completely.

Isn't this file already skipped on Windows?

vdemeester · 2018-02-27T16:01:05Z

cli/command/trust/key_load_test.go

 	"testing"

+	"github.com/gotestyourself/gotestyourself/skip"
+


vdemeester · 2018-02-27T16:02:17Z

cli/command/stack/swarm/deploy_composefile_test.go

 			expectedMsg:  "Unexpected",
 		},
 		{
+			// FIXME(vdemeester) that doesn't work under windows, the check needs to be smarter


This tests fails under Windows because of : 7f53c99#r27815425

I think the info endpoint could be queried instead of making the assumption that daemon os/platform == client os/platform

definitely, but probably need to be "skipped" for now, and follow-up on a fix PR for that problem only 👼

codecov-io · 2018-02-27T16:08:14Z

Codecov Report

Merging #906 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #906      +/-   ##
==========================================
+ Coverage   53.26%   53.32%   +0.05%     
==========================================
  Files         259      258       -1     
  Lines       16432    16374      -58     
==========================================
- Hits         8753     8731      -22     
+ Misses       7115     7079      -36     
  Partials      564      564

thaJeztah

LGTM, but left some comments 😅

thaJeztah · 2018-02-27T16:37:01Z

cli/command/formatter/history_test.go

 func TestHistoryContext_CreatedSince(t *testing.T) {
+	dateStr := "2009-11-10T23:00:00Z"
+	if runtime.GOOS == "windows" {
+		dateStr = "2009-11-11T00:00:00+01:00"


arf

Is this dependent on system configuration?

I'm actually not sure 😓 cc @simonferquel @johnstep

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).

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.

thaJeztah · 2018-02-27T16:46:35Z

cli/command/plugin/create_test.go

 		{
 			args:          []string{"plugin-foo", "nonexistent_context_dir"},
-			expectedError: "no such file or directory",
+			expectedError: noSuchFile,


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

I think there is an os.IsNotFound() or os.IsFileNotFound function that would be prettier

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

thaJeztah · 2018-02-27T16:48:34Z

cli/command/image/build_linux_test.go

+)
+
+func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	skip.IfCondition(t, runtime.GOOS == "windows", "uid and gid not relevant on windows")


Isn't this file already skipped on Windows?

Some of them are skipped for now (because the feature is not supported or needs more work), some of them are fixed. Signed-off-by: Vincent Demeester <[email protected]>

dnephin · 2018-02-27T18:34:57Z

cli/command/image/build_linux_test.go

@@ -0,0 +1,54 @@
+//+build linux


Could we use skip instead of the build directive? Or does this not build at all on windows?

@dnephin it doesn't build at all on windows — there was a skip before 😓

dnephin · 2018-02-27T18:38:19Z

cli/command/trust/key_load_test.go

 }

 func TestLoadKeyFromPath(t *testing.T) {
+	skip.If(t, func() bool { return runtime.GOOS == "windows" })


Instead of the func() bool here you can use IfCondition, or update gotestyourself to 1.3.0 so that If accepts the bool.

Maybe add a reason why as well?

dnephin · 2018-02-27T18:40:04Z

cli/compose/loader/full-struct_test.go

 				{Source: "/opt/data", Target: "/var/lib/mysql", Type: "bind"},
 				{Source: workingDir, Target: "/code", Type: "bind"},
-				{Source: workingDir + "/static", Target: "/var/www/html", Type: "bind"},
+				{Source: filepath.Join(workingDir, "/static"), Target: "/var/www/html", Type: "bind"},


remove the / from /static ? Also on opt below.

yes the / is weird

oups, I forgot to remove that one 🤦‍♂️

simonferquel · 2018-02-28T09:52:51Z

cli/command/formatter/history_test.go

 func TestHistoryContext_CreatedSince(t *testing.T) {
+	dateStr := "2009-11-10T23:00:00Z"
+	if runtime.GOOS == "windows" {
+		dateStr = "2009-11-11T00:00:00+01:00"


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).

simonferquel · 2018-02-28T09:53:57Z

cli/command/image/build_linux_test.go

+)
+
+func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	dest := fs.NewDir(t, "test-build-context-dest")


Should'nt we run this test on Darwin as well ?

@simonferquel yes, this is also on my TODO (but I should open an issue to track the work on that)

simonferquel · 2018-02-28T09:55:14Z

cli/command/plugin/create_test.go

 		{
 			args:          []string{"plugin-foo", "nonexistent_context_dir"},
-			expectedError: "no such file or directory",
+			expectedError: noSuchFile,


I think there is an os.IsNotFound() or os.IsFileNotFound function that would be prettier

simonferquel · 2018-02-28T09:58:00Z

cli/command/stack/swarm/deploy_composefile_test.go

 			expectedMsg:  "Unexpected",
 		},
 		{
+			// FIXME(vdemeester) that doesn't work under windows, the check needs to be smarter


I think the info endpoint could be queried instead of making the assumption that daemon os/platform == client os/platform

simonferquel · 2018-02-28T09:59:39Z

cli/compose/loader/full-struct_test.go

 				{Source: "/opt/data", Target: "/var/lib/mysql", Type: "bind"},
 				{Source: workingDir, Target: "/code", Type: "bind"},
-				{Source: workingDir + "/static", Target: "/var/www/html", Type: "bind"},
+				{Source: filepath.Join(workingDir, "/static"), Target: "/var/www/html", Type: "bind"},


yes the / is weird

simonferquel · 2018-02-28T09:59:51Z

cli/compose/loader/full-struct_test.go

 				{Source: homeDir + "/configs", Target: "/etc/configs/", Type: "bind", ReadOnly: true},
 				{Source: "datavolume", Target: "/var/lib/mysql", Type: "volume"},
-				{Source: workingDir + "/opt", Target: "/opt", Consistency: "cached", Type: "bind"},
+				{Source: filepath.Join(workingDir, "/opt"), Target: "/opt", Consistency: "cached", Type: "bind"},


🤦‍♂️

vdemeester · 2018-02-28T14:31:57Z

Closing this one as I'm putting the commit in #905 (and I'll take comments into account there)

vdemeester requested review from dnephin, simonferquel and thaJeztah February 27, 2018 15:59

GordonTheTurtle added the status/0-triage label Feb 27, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Feb 27, 2018

vdemeester commented Feb 27, 2018

View reviewed changes

cli/command/trust/key_load_test.go Outdated

"testing"

"github.com/gotestyourself/gotestyourself/skip"

Copy link

Collaborator Author

vdemeester Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh 😓

vdemeester commented Feb 27, 2018

View reviewed changes

vdemeester force-pushed the fix-some-windows-unit-tests branch from e39587d to f7f4172 Compare February 27, 2018 16:08

thaJeztah approved these changes Feb 27, 2018

View reviewed changes

Fixes some unit tests to be able to run them on windows

8da068f

Some of them are skipped for now (because the feature is not supported or needs more work), some of them are fixed. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the fix-some-windows-unit-tests branch from f7f4172 to 8da068f Compare February 27, 2018 17:03

dnephin reviewed Feb 27, 2018

View reviewed changes

vdemeester mentioned this pull request Feb 28, 2018

Add appveyor setup to build and unit test #905

Merged

simonferquel suggested changes Feb 28, 2018

View reviewed changes

vdemeester closed this Feb 28, 2018

vdemeester deleted the fix-some-windows-unit-tests branch June 26, 2018 11:58

Conversation

vdemeester commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Feb 27, 2018 •

edited

Loading