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: remove bash dependency of TestRunSlowStdoutConsumer #10804

Merged
merged 1 commit into from Feb 16, 2015

Conversation

Projects
None yet
7 participants
@ahmetb
Contributor

ahmetb commented Feb 14, 2015

This makes this test case run on msys bash on windows or
cmd.exe. On windows, os.Exec("/bin/bash") simply fails because
that's not the correct abspath.

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

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 14, 2015

LGTM

@duglin

This comment has been minimized.

Contributor

duglin commented Feb 14, 2015

As these changes are being made should we be adding some kind of validation tests to make sure people don't add them back in?

And perhaps a page telling people what constructs to avoid and which to use ?

-Doug

Sent from my iPhone

On Feb 14, 2015, at 5:17 PM, Tibor Vass notifications@github.com wrote:

LGTM


Reply to this email directly or view it on GitHub.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 14, 2015

I'm not sure how we can validate that. If you're referring to bash usage, that'd be an obvious failure in Windows CI.

@duglin

This comment has been minimized.

Contributor

duglin commented Feb 15, 2015

I just don’t want you to have to continually make these kinds of edits over and over - nor wait for a windows build - so if we can add something like the validate-gofmt tests then we’ll know if someone uses something they shouldn’t

-Doug

On Feb 14, 2015, at 6:45 PM, Ahmet Alp Balkan notifications@github.com wrote:

I'm not sure how we can validate that. If you're referring to bash usage, that'd be an obvious failure in Windows CI.

—Alp
http://alp.im

On Sat, Feb 14, 2015 at 3:37 PM, Doug Davis notifications@github.com
wrote:

As these changes are being made should we be adding some kind of validation tests to make sure people don't add them back in?
And perhaps a page telling people what constructs to avoid and which to use ?
-Doug
Sent from my iPhone

On Feb 14, 2015, at 5:17 PM, Tibor Vass notifications@github.com wrote:

LGTM


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub:
#10804 (comment)

Reply to this email directly or view it on GitHub #10804 (comment).

@@ -2617,7 +2617,7 @@ func TestRunVolumesCleanPaths(t *testing.T) {
func TestRunSlowStdoutConsumer(t *testing.T) {
defer deleteAllContainers()
c := exec.Command("/bin/bash", "-c", dockerBinary+` run --rm -i busybox /bin/sh -c "dd if=/dev/zero of=/foo bs=1024 count=2000 &>/dev/null; catv /foo"`)
c := exec.Command(dockerBinary, "run", "--rm", "-i", "busybox", "/bin/sh", "-c", "dd if=/dev/zero of=/foo bs=1024 count=2000 &>/dev/null; catv /foo")

This comment has been minimized.

@tianon

tianon Feb 15, 2015

Member

&> is a bash-ism -- this ought to be > /dev/null 2>&1 instead

This comment has been minimized.

@tiborvass

tiborvass Feb 15, 2015

Collaborator

nice catch!

This comment has been minimized.

@ahmetb

ahmetb Feb 15, 2015

Contributor

fixing while I'm at it.

This comment has been minimized.

@ahmetb

ahmetb Feb 15, 2015

Contributor

Fixed this with dd if=/dev/zero of=/dev/stdout bs=1024 count=2000 | catv because at the end test wants to stream just 1024 * 2 * 2000 chars.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Feb 15, 2015

I wonder why this was going through bash to begin with.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 15, 2015

@cpuguy83 hehe no idea. many tests using binary stdout results from dockerBinary were using bash. I don't think there's a particular benefit of using bash in these cases.

@@ -2617,7 +2617,7 @@ func TestRunVolumesCleanPaths(t *testing.T) {
func TestRunSlowStdoutConsumer(t *testing.T) {
defer deleteAllContainers()
c := exec.Command("/bin/bash", "-c", dockerBinary+` run --rm -i busybox /bin/sh -c "dd if=/dev/zero of=/foo bs=1024 count=2000 &>/dev/null; catv /foo"`)
c := exec.Command(dockerBinary, "run", "--rm", "-i", "busybox", "/bin/sh", "-c", "dd if=/dev/zero of=/dev/stdout bs=1024 count=2000 | catv")

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 16, 2015

Contributor

Can we get rid of "-i" as well since we aren't doing anything with stdin?

This comment has been minimized.

@ahmetb

ahmetb Feb 16, 2015

Contributor

@cpuguy83 fair point, I didn't know stdout was getting attached without -i. :)

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Feb 16, 2015

LGTM just one nit.

integration-cli: remove bash dependency of TestRunSlowStdoutConsumer
This makes this test case run on msys bash on windows or
cmd.exe.

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

This comment has been minimized.

Contributor

jessfraz commented Feb 16, 2015

LGTM

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

Jessie Frazelle
Merge pull request #10804 from ahmetalpbalkan/win-cli/TestRunSlowStdo…
…utConsume-fix

integration-cli: remove bash dependency of TestRunSlowStdoutConsumer

@jessfraz jessfraz merged commit 110ce4f into moby:master Feb 16, 2015

1 check passed

janky Jenkins build Docker-PRs 1160 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:win-cli/TestRunSlowStdoutConsume-fix branch Feb 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment