Code review comment for lp://staging/~jameinel/juju-core/timing-test

Reviewers: mp+169155_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: Make a test deterministic

TestGatherDescriptionsInParallel was timing dependent, and occasionally
failed
on my system. Rather than just increasing the timeout arbitrarily, I
changed
the code to be more 'event' based. Each plugin requires another plugin
to
have started before that plugin will return. Thus, if we don't start the
plugins in parallel, the plugin will deadlock.

Because the failure mode is 'deadlock', I wrapped the call in a select
timeout.
This means that if it *does* fail, we have a garbage goroutine and
process
lying around (but it should never fail).

On the plus side, this makes it actually run faster, since each
subprocess doesn't
waste time sleeping just so we can measure it. (from 350ms down to
normally
around 15ms.)

This is the only test that was failing (based on occasional timing
hiccups)
for me, which allows us to have this in tarmac.

https://code.launchpad.net/~jameinel/juju-core/timing-test/+merge/169155

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10234047/

Affected files:
   A [revision details]
   M cmd/juju/plugin_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: cmd/juju/plugin_test.go
=== modified file 'cmd/juju/plugin_test.go'
--- cmd/juju/plugin_test.go 2013-05-22 21:47:11 +0000
+++ cmd/juju/plugin_test.go 2013-06-13 10:32:38 +0000
@@ -92,21 +92,31 @@
  }

  func (suite *PluginSuite) TestGatherDescriptionsInParallel(c *C) {
- suite.makeFullPlugin(PluginParams{Name: "foo", Sleep: 100 *
time.Millisecond})
- suite.makeFullPlugin(PluginParams{Name: "bar", Sleep: 150 *
time.Millisecond})
- suite.makeFullPlugin(PluginParams{Name: "baz", Sleep: 300 *
time.Millisecond})
- suite.makeFullPlugin(PluginParams{Name: "error", ExitStatus: 1, Sleep:
100 * time.Millisecond})
- suite.makeFullPlugin(PluginParams{Name: "slow", Sleep: 200 *
time.Millisecond})
-
- start := time.Now()
- results := GetPluginDescriptions()
- elapsed := time.Since(start)
-
- // 300 for baz above + 50ms wiggle room
- expectedDuration := 350 * time.Millisecond
-
- c.Assert(results, HasLen, 5)
- c.Check(elapsed, DurationLessThan, expectedDuration)
+ // Each plugin depends on another one being started before they will
complete.
+ // Thus if we don't start them in parallel, we would deadlock
+ suite.makeFullPlugin(PluginParams{Name: "foo", Creates: "foo",
DependsOn: "bar"})
+ suite.makeFullPlugin(PluginParams{Name: "bar", Creates: "bar",
DependsOn: "baz"})
+ suite.makeFullPlugin(PluginParams{Name: "baz", Creates: "baz",
DependsOn: "error"})
+ suite.makeFullPlugin(PluginParams{Name: "error", ExitStatus: 1,
Creates: "error", DependsOn: "foo"})
+
+ // If the code was wrong, GetPluginDescriptions would deadlock,
+ // so timeout after a short while
+ resultChan := make(chan []PluginDescription)
+ go func() {
+ resultChan <- GetPluginDescriptions()
+ }()
+ // 10 seconds is arbitrary but should always be generously long. Test
+ // actually only takes about 15ms in practice. But 10s allows for system
hiccups, etc.
+ waitTime := 10 * time.Second
+ var results []PluginDescription
+ select {
+ case results = <-resultChan:
+ break
+ case <-time.After(waitTime):
+ c.Fatalf("Took too more than %fs to complete.", waitTime.Seconds())
+ }
+
+ c.Assert(results, HasLen, 4)
   c.Assert(results[0].name, Equals, "bar")
   c.Assert(results[0].description, Equals, "bar description")
   c.Assert(results[1].name, Equals, "baz")
@@ -115,8 +125,6 @@
   c.Assert(results[2].description, Equals, "error occurred
running 'juju-error --description'")
   c.Assert(results[3].name, Equals, "foo")
   c.Assert(results[3].description, Equals, "foo description")
- c.Assert(results[4].name, Equals, "slow")
- c.Assert(results[4].description, Equals, "slow description")
  }

  func (suite *PluginSuite) TestHelpPluginsWithNoPlugins(c *C) {
@@ -169,15 +177,24 @@
  type PluginParams struct {
   Name string
   ExitStatus int
- Sleep time.Duration
+ Creates string
+ DependsOn string
  }

  const pluginTemplate = `#!/bin/bash

  if [ "$1" = "--description" ]; then
- sleep {{.Sleep.Seconds}}
+ if [ -n "{{.Creates}}" ]; then
+ touch "{{.Creates}}"
+ fi
+ if [ -n "{{.DependsOn}}" ]; then
+ # Sleep 10ms while waiting to allow other stuff to do work
+ while [ ! -e "{{.DependsOn}}" ]; do sleep 0.010; done
+ fi
    echo "{{.Name}} description"
    exit {{.ExitStatus}}
+else
+ echo "No --description" >2
  fi

  if [ "$1" = "--help" ]; then
@@ -195,7 +212,14 @@
   // Create a new template and parse the plugin into it.
   t := template.Must(template.New("plugin").Parse(pluginTemplate))
   content := &bytes.Buffer{}
+ filename := testing.HomePath("juju-" + params.Name)
+ // Create the files in the temp dirs, so we don't pollute the working
space
+ if params.Creates != "" {
+ params.Creates = testing.HomePath(params.Creates)
+ }
+ if params.DependsOn != "" {
+ params.DependsOn = testing.HomePath(params.DependsOn)
+ }
   t.Execute(content, params)
- filename := testing.HomePath("juju-" + params.Name)
   ioutil.WriteFile(filename, content.Bytes(), 0755)
  }