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 timeout dependency on TestEventsUntag #10798

Merged
merged 1 commit into from Mar 4, 2015

Conversation

Projects
None yet
7 participants
@ahmetb
Contributor

ahmetb commented Feb 14, 2015

TestEventsUntag requires a timeout command which does not
exist on OS X or Windows (in fact, windows has a totally different
timeout program and this test was accidentally using it, which is
way worse in terms of finding out why it was failing).

  • Created runCommandWithOutputForDuration.
    This entirely replaces runDockerCommandWithTimeout and
    removes dependency to timeout command.
  • Turned out runDockerCommandWithTimeout is only used by
    dockerCmdWithTimeout/dockerCmdInDirWithTimeout.
  • Apparently dockerCmdWithTimeout/dockerCmdInDirWithTimeout
    are dead code. I cleaned them while I'm at it.
  • Made runDockerCommandWithTimeout reuse runDockerCommandForDuration.
    This fixes its bugs mentioned above.

TestEventsUntag works now on Windows.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Label: #windows
cc: @unclejack @gsalgado @jfrazelle @icecrime @tiborvass

@unclejack

This comment has been minimized.

Contributor

unclejack commented Feb 14, 2015

@ahmetalpbalkan dockerCmdInDirWithTimeout and dockerCmdWithTimeout were there to be used in the future as that was a set of utility functions to be used in the tests.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 14, 2015

@unclejack oh I didn't know that. I restored them and reused the new function to replace runDockerCommandWithTimeout with a simple and better implementation that could return the captured output even though execution times out (and still returns ErrCmdTimeout).

integration-cli: remove timeout dependency on TestEventsUntag
TestEventsUntag requires a `timeout` command which does not
exist on OS X or Windows (in fact, windows has a totally different
timeout program and this test was accidentally using it).

- Created runCommandWithOutputForDuration.
  This entirely replaces runDockerCommandWithTimeout and
  removes dependency to `timeout` command.
- Made runDockerCommandWithTimeout reuse runDockerCommandForDuration.

TestEventsUntag works now on Windows.

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

This comment has been minimized.

Collaborator

tiborvass commented Feb 14, 2015

@ahmetalpbalkan the two functions are redundant, I would keep the one with 3 return arguments, (although tbh, we NEVER use exitcode...), that way no need for timedOut, since if we want to know if it timed out we just check for the error.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 14, 2015

@tiborvass this specific test case doesn't care if the process didn't finish within time because it runs indefinitely. From caller perspective if err==ErrTimedOut check is way more uglier than having two neat helper methods with less footprint to me.

Also there's no reason to not to use exitCode for debugging. We should probably add it to Failf of this test.

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 14, 2015

@ahmetalpbalkan the exitCode is almost always 1, that's why it's useless, but this debate is orthogonal.
In practice, we will just do if err != nil { and print out the error potentially with some additional information. The error will say it's timeout. So we don't even need to check specifically for ErrTimedOut.

I still vote for just having one function that returns the error. I really don't see us using timedOut in another case than just erroring out.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 14, 2015

@tiborvass I still will be needing a function for this test case that does not return an ErrTimedOut when failed, but returns the err if cmd.Start fails. So are you saying that one function should not return ErrTimedOut at all and we don't need to find out if we timed out, ever?

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 20, 2015

@tiborvass ping on this. any objections on runDockerCommandWithTimeout implementation?

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 23, 2015

1 function is easier like @tiborvass said

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 23, 2015

1 function is easier like @tiborvass said

@jfrazelle I'm still trying to understand this. Pick one:

  1. a function that returns error when executed command is not exited within allotted time
  2. a function that executes given command for specified period of time and collects its output

we can achieve both with a big reuse. I can't see why we can't have both.

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 24, 2015

I think no. 1

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 24, 2015

@jfrazelle ok then in this case we'll discard the err... the test case code will look like

out, err := runWithTimeout(cmd, timeout)
if err == errTimedOut {
    // ignore
}
else {
     //actual err
} 

whereas we could do

out, err := runForDuration(cmd, timeout) // which actually uses runWithTimeout internally, or the other way around
if err != nil {
    // actual err
} 

what's wrong with having both runWithTimeout/runForDuration which one of them is just a thin wrapper on top of another?

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 24, 2015

I guess that is fine since they have 2 different purposes LGTM

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 24, 2015

yea, totally. and this doesn't break behavior/signature for runCommandWithOutputAndTimeout at all and yet reuses the new method underneath.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 24, 2015

any other LGTMs?

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 25, 2015

ping @tiborvass do you approve

eventsCmd := exec.Command("timeout", "0.2", dockerBinary, "events", "--since=1")
out, _, _ := runCommandWithOutput(eventsCmd)
eventsCmd := exec.Command(dockerBinary, "events", "--since=1")
out, exitCode, _, err := runCommandWithOutputForDuration(eventsCmd, time.Duration(time.Millisecond*200))

This comment has been minimized.

@tiborvass

tiborvass Feb 25, 2015

Collaborator

@ahmetalpbalkan Why don't we check for timeout here ? I know the original code didn't but I wonder whether we should or not.

This comment has been minimized.

@ahmetb

ahmetb Feb 25, 2015

Contributor

@tiborvass the point of this test is to start and indefinitely-running program and keep it running for specified duration, collect its output and then kill it. Timing out is not an error here, it is exactly what we do on purpose.

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 25, 2015

LGTM

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 25, 2015

@jfrazelle this test was passing a while ago. started to fail again:

https://jenkins.dockerproject.com/job/Windows-PRs/83/console
=== RUN TestEventsUntag
--- FAIL: TestEventsUntag (0.99s)
docker_cli_events_test.go:30: event should be untag, not "2015-02-25T21:22:58.000000000Z 0f8cf7f4c6413a4c488a53e9559543e1fed5e3e0fbc53805d0988b7585d5c8ba: (from busybox:latest) create"

does it smell like #10904?

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 25, 2015

im telling you the events ones are the worst, it might be the same issue,
try moving it somewhere else as a first step lol

On Wed, Feb 25, 2015 at 1:38 PM, Ahmet Alp Balkan notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle this test was passing a while
ago. started to fail again:

https://jenkins.dockerproject.com/job/Windows-PRs/83/console
=== RUN TestEventsUntag
--- FAIL: TestEventsUntag (0.99s)
docker_cli_events_test.go:30: event should be untag, not
"2015-02-25T21:22:58.000000000Z
0f8cf7f4c6413a4c488a53e9559543e1fed5e3e0fbc53805d0988b7585d5c8ba: (from
busybox:latest) create"

does it smell like #10904 #10904?


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

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 25, 2015

rebuilding just to see if it's a race.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 25, 2015

OK this is failing consistently on windows CI machine (works fine on my machine) I'll investigate sometime. Could be because we're pulling the busybox container in test runtime. Let's keep this open.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 4, 2015

This test still fixes the problem (works on my machine). But windows CI is now suffering from a totally different problem in all tests using docker events introduced recently (can't find the root cause though).

This will be fixing the problem anyway once we solve the "events" problems on the CI machines in general. There's nothing stops us from reviewing and merging? @tianon @jfrazelle @tiborvass

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Mar 4, 2015

Its probably picking up events from the test above it, considering no one has changed that specific test

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Mar 4, 2015

but seeing as its unrelated we can merge this

jessfraz pushed a commit that referenced this pull request Mar 4, 2015

Jessie Frazelle
Merge pull request #10798 from ahmetalpbalkan/win-cli/TestEventsUntag…
…-fix

integration-cli: Remove timeout dependency on TestEventsUntag

@jessfraz jessfraz merged commit a8b699f into moby:master Mar 4, 2015

1 of 2 checks passed

windows Jenkins build Windows-PRs 85 has failed
Details
janky Jenkins build Docker-PRs 1082 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:win-cli/TestEventsUntag-fix branch Mar 4, 2015

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