worker/uniter/runner/debug: better test logging #6402

Merged
merged 2 commits into from Oct 7, 2016

Conversation

Projects
None yet
4 participants
Owner

howbazaar commented Oct 7, 2016

This work was in response to a test timeout in the debug package.

After much examination it was determined that the test timed out because RunHook failed to start properly, and the case statement in the first select was only calling c.Error. Error only marks the test as failing, it doesn't actually cause the test to stop, so when it did fail there, the test kept looping.

Extra testing.LongWait calls added to each select as a firm backdoor. Now the ServerSession captures the output of the hook execution in order to allow the test to write it out in the failure case.

mjs approved these changes Oct 7, 2016

@@ -0,0 +1,11 @@
+// Copyright 2013 Canonical Ltd.
@mjs

mjs Oct 7, 2016

Contributor

2016

worker/uniter/runner/debug/server.go
@@ -40,6 +44,9 @@ func (s *ServerSession) RunHook(hookName, charmDir string, env []string) error {
cmd.Env = env
cmd.Dir = charmDir
cmd.Stdin = bytes.NewBufferString(debugHooksServerScript)
+ var combinedOutput bytes.Buffer
@axw

axw Oct 7, 2016

Member

This means a hook's output won't show up until it completes, which can take an arbitrarily long amount of time. This may have an impact on usability of debug-hooks.

Please either add Stdout and Stderr parameters to HookContext/ServerSession and use them, or modify echocommand to do what you need.

@howbazaar

howbazaar Oct 7, 2016

Owner

I've reworked how we get the output for the test now, can you take another look plz?

axw approved these changes Oct 7, 2016

Owner

howbazaar commented Oct 7, 2016

$$merge$$

Contributor

jujubot commented Oct 7, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit ffd6121 into juju:master Oct 7, 2016

@howbazaar howbazaar deleted the howbazaar:better-test-logging branch Oct 7, 2016

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