integration-cli: remove bash dependency of TestRunSlowStdoutConsumer #10804
Conversation
|
LGTM |
|
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
|
|
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. |
|
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
|
| @@ -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") | |||
There was a problem hiding this comment.
&> is a bash-ism -- this ought to be > /dev/null 2>&1 instead
There was a problem hiding this comment.
fixing while I'm at it.
There was a problem hiding this comment.
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.
|
I wonder why this was going through bash to begin with. |
|
@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") | |||
There was a problem hiding this comment.
Can we get rid of "-i" as well since we aren't doing anything with stdin?
There was a problem hiding this comment.
@cpuguy83 fair point, I didn't know stdout was getting attached without -i. :)
|
LGTM just one nit. |
This makes this test case run on msys bash on windows or cmd.exe. Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
|
LGTM |
…utConsume-fix integration-cli: remove bash dependency of TestRunSlowStdoutConsumer
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:
#windowscc: @unclejack @cpuguy83 @tianon @tiborvass