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

integration-cli: use pipes for save/load tests #10806

Merged
merged 1 commit into from
Feb 19, 2015
Merged

integration-cli: use pipes for save/load tests #10806

merged 1 commit into from
Feb 19, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 14, 2015

This removes bash dependency from save/load integration tests.
It used to call /bin/bash -c 'c:\...\docker.exe' which is not valid
when executed on Windows.

Also removed usage of tempdirs and temp files for saving/loading
repos. All are now done using in-memory pipes and buffers.

Created runCommandPipelineWithOutput(...cmd) helper to replace the
/bin/bash -c 'a | b | c' using pipes and returning output from
last command in the pipeline. This makes the code even shorter
and readable.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Label: #windows
cc: @unclejack @vbatts @vieux @jfrazelle @a-ba @tianon

@tianon
Copy link
Member

tianon commented Feb 16, 2015

LGTM 👍

I want this for #10750 now! 😄

@tianon
Copy link
Member

tianon commented Feb 17, 2015

--- FAIL: TestSaveSingleTag (0.46s)
    docker_cli_save_load_test.go:152: failed to save repo with image ID and 'repositories' file: , exit status 1
[PASSED]: save - save a image by ID
[PASSED]: save - save a repo using -o && load a repo using -i
--- FAIL: TestSaveMultipleNames (0.16s)
    docker_cli_save_load_test.go:288: failed to save multiple repos: , exit status 1
--- FAIL: TestSaveRepoWithMultipleImages (1.80s)
    docker_cli_save_load_test.go:349: achive does not contains the right layers: got [], expected [022602255a522cdc56c90787ef95f35b6c29ea43aa49f23e11c0b982ac31f9a3 522d1604e1a0aaad296fff54dab5e062e1d2892b8542095fae0f24874e785797 7fd0431ef4468370712b452bbdc5c7eef13b71796c8f6ad4157ec6bfce35f0b5 aae965979569e60ffd92ac3e9ef9e469a1b35c6f0a690e9ba912450db608af43 ba11eb266ed9a73e7a2d321e061735972eecbb4c24542082c21838eb9c635164]

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 17, 2015

@tianon ugh. apparently we can do things can pass on windows and fail on linux... I'll look this sometime. lmk if you know you can see something right away.

out, _, err = runCommandPipelineWithOutput(
exec.Command(dockerBinary, "save", fmt.Sprintf("%v:latest", repoName)),
exec.Command("tar", "t"),
exec.Command("grep", "-E", fmt.Sprintf("'(^repositories$|%v)'", cleanedImageID)))
Copy link
Member

Choose a reason for hiding this comment

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

OMG it's the single quotes here. Drop the single quotes inside the quotes here and we should be golden.

Copy link
Member

Choose a reason for hiding this comment

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

If that doesn't work, it might be worth scripting the output of tar t directly (essentially implementing the grep in Go instead of shelling out), but I think getting the test converted as-is first is a good first step, personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tianon are you sure it's the single quotes? all other tests regarding this change are failing, too. also the quotes were there and we're basically keeping it in this change. I doubt that's the problem.

Copy link
Member

Choose a reason for hiding this comment

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

I'm positive it's the quotes, because they were there as an artifact of the shell syntax to ensure this argument was passed correctly, which exec.Command already does for us regardless of the shell.

Copy link
Member

Choose a reason for hiding this comment

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

So, why isn't this test failing, since it still has the quotes? 😟

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tianon you're probably looking at an older commit. see b81105e.

This removes `bash` dependency from save/load integration tests.
It used to call `/bin/bash -c 'c:\...\docker.exe'` which is not valid.
Also removed usage of tempdirs and temp files for saving/loading
repos. All are now done using in-memory pipes and buffers.

Created `runCommandPipelineWithOutput` helper to replace the
`/bin/bash -c 'a | b | c'` using pipes and returning output from
last command in the pipeline. This makes the code even shorter
and readable.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 18, 2015

those are fixed now. yes it was 's around some args.

@tianon
Copy link
Member

tianon commented Feb 19, 2015

LGTM

@jessfraz
Copy link
Contributor

Woooohooo it is time LGTM

jessfraz pushed a commit that referenced this pull request Feb 19, 2015
integration-cli: use pipes for save/load tests
@jessfraz jessfraz merged commit cd9769c into moby:master Feb 19, 2015
@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 19, 2015

😂 👯👯👯👯

@ahmetb ahmetb deleted the win-cli/TestSave-fix branch February 19, 2015 06:44
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.

None yet

6 participants