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

integration-cli: Generate unix-style volume paths for tests #10794

Merged
merged 1 commit into from Feb 19, 2015

Conversation

Projects
None yet
8 participants
@ahmetb
Contributor

ahmetb commented Feb 14, 2015

Some tests in docker_api_containers_test.go assume the
docker daemon is running at the same machine as the cli
and uses ioutil.TempDir to create temp dirs and use them
in the test.

On windows ioutil.TempDir and os.TempDir would create win-style
paths and pass them to daemon. Instead, I hardcoded /tmp/test.{{randString}}
and test generates some random path manually and allow daemon to create
the directory on daemon-side.

Fixes tests:

  • TestContainerApiStartVolumeBinds
  • TestContainerApiStartDupVolumeBinds
  • TestVolumesFromHasPriority

Downside:

  • Does not clean the temp dirs generated on the remote daemon machine unless delete container deletes them.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @unclejack @cpuguy83 @estesp @jfrazelle
Label: #windows

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 14, 2015

https://jenkins.dockerproject.com/job/Windows-PRs/36/console
=== RUN TestContainerApiStartVolumeBinds
[PASSED]: container REST API - check volume binds on start
--- PASS: TestContainerApiStartVolumeBinds (1.21s)
=== RUN TestContainerApiStartDupVolumeBinds
[PASSED]: container REST API - check for duplicate volume binds error on start
--- PASS: TestContainerApiStartDupVolumeBinds (0.94s)
=== RUN TestVolumesFromHasPriority
[PASSED]: container REST API - check VolumesFrom has priority
--- PASS: TestVolumesFromHasPriority (1.92s) 👍

t.Fatal(err)
}
bindPath := path.Join("/tmp", fmt.Sprintf("test.%s", makeRandomString(10)))

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 15, 2015

Contributor

Shouldn't this be filepath, not path?

Nevermind, this is on Windows, so yeah, path is what we want :)

This comment has been minimized.

@thaJeztah

thaJeztah Feb 15, 2015

Member

^^ so, this might confuse people, because they think it should be replaced by filepath (because that's usually the right way); I think a comment should be added to these lines to explain why this is used and to prevent people from refactoring them by accident.

This comment has been minimized.

@duglin

duglin Feb 15, 2015

Contributor

@cpuguy83 why would filepath be wrong?

This comment has been minimized.

@thaJeztah

thaJeztah Feb 15, 2015

Member

If I understood this PR correctly: because of #10794 (comment); filepath would attempt to create a Windows-style path (C:\temp) on the daemon, but the daemon is running on linux

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 15, 2015

Contributor

Yep. Generating the path on Windows, but the path is for Linux.

This comment has been minimized.

@duglin

duglin Feb 15, 2015

Contributor

one thing that I've been wondering about... in all of my windows programming I've always been able to use / instead of \ in my paths - is this not true for golang? Just wondering if we really need to use filepath.Join() when we're on windows at all.

This comment has been minimized.

@thaJeztah

thaJeztah Feb 15, 2015

Member

I like the idea for some kind of utility to prevent people tripping over this (something like daemon.filepath()?) then no comments would be required, and it would keep working if eventually a Windows Daemon arrives (just thinking out loud here :))

This comment has been minimized.

@ahmetb

ahmetb Feb 15, 2015

Contributor

@duglin normally it might work a little bit, but you're passing c:\users\xxx\appdata\temp\test.123 to docker daemon. no way that's working with colon symbol and all :)

t.Fatal(err)
}
defer os.Remove(bindPath2)
bindPath1 := path.Join("/tmp", fmt.Sprintf("test1.%s", makeRandomString(10)))

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 15, 2015

Contributor

Same, filepath

t.Fatal(err)
}
bindPath := path.Join("/tmp", fmt.Sprintf("test.%s", makeRandomString(10)))

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 15, 2015

Contributor

filepath

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Feb 15, 2015

So, we need paths a lot in the tests. Maybe we should make a helper for creating temp paths like this since we can't use the normal mechanisms.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 17, 2015

@cpuguy83 extracted into a helper.

integration-cli: generate unix-style volume paths
Some tests in `docker_api_containers_test.go` assume the
docker daemon is running at the same machine as the cli
and uses `ioutil.TempDir` to create temp dirs and use them
in the test.

On windows ioutil.TempDir and os.TempDir would create win-style
paths and pass them to daemon. Instead, I hardcoded `/tmp/` and
generate some random path manually and allow daemon to create
the directory.

Fixes tests:
- TestContainerApiStartVolumeBinds
- TestContainerApiStartDupVolumeBinds
- TestVolumesFromHasPriority

Downside:
- Does not clean the temp dirs generated on the remote daemon
  machine unless delete container deletes them.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Feb 17, 2015

Sweet, LGTM

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 19, 2015

kewl LGTM

jessfraz pushed a commit that referenced this pull request Feb 19, 2015

Jessie Frazelle
Merge pull request #10794 from ahmetalpbalkan/win-cli/TestContainerAp…
…i-volume-path-fix

integration-cli: Generate unix-style volume paths for tests

@jessfraz jessfraz merged commit 47e9f90 into moby:master Feb 19, 2015

1 check passed

janky Jenkins build Docker-PRs 1201 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:win-cli/TestContainerApi-volume-path-fix branch Feb 19, 2015

brahmaroutu added a commit to brahmaroutu/docker that referenced this pull request Feb 19, 2015

integration-cli: seed rand in makeRandomString
Current uses of `makeRandomString` is to create really
long strings. In moby#10794, I used them to create nearly-unique
unix paths for the daemon. Although collions are harmless and
don't fail the tests, this prevents the same strings from being
created consistently in every run by seeding rand.Random.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>

spf13 added a commit to spf13/docker that referenced this pull request Mar 14, 2015

integration-cli: seed rand in makeRandomString
Current uses of `makeRandomString` is to create really
long strings. In moby#10794, I used them to create nearly-unique
unix paths for the daemon. Although collions are harmless and
don't fail the tests, this prevents the same strings from being
created consistently in every run by seeding rand.Random.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment