Merge into trunk : timing-test : Code : juju-core

Status:	Merged
Merge reported by:	John A Meinel
Merged at revision:	not available
Proposed branch:	lp://staging/~jameinel/juju-core/timing-test
Merge into:	lp://staging/~juju/juju-core/trunk
Diff against target:	104 lines (+46/-20) 1 file modified cmd/juju/plugin_test.go (+46/-20)
To merge this branch:	bzr merge lp://staging/~jameinel/juju-core/timing-test
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2013-06-13	Pending
Review via email: mp+169155@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/10234047/

Revision history for this message

John A Meinel (jameinel) wrote on 2013-06-13:

#

Download full text (5.4 KiB)

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()...

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: tim.penhey@canonical.com-20130613012358-ujvlprm6pfo2r7yn
+New revision: john@arbash-meinel.com-20130613103238-p072b32r5tbzr9l0

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)
  }

Revision history for this message

William Reade (fwereade) wrote on 2013-06-13:

#

LGTM

https://codereview.appspot.com/10234047/

Revision history for this message

Martin Packman (gz) wrote on 2013-06-13:

#

LGTM

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go
File cmd/juju/plugin_test.go (left):

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go#oldcode99
cmd/juju/plugin_test.go:99: suite.makeFullPlugin(PluginParams{Name:
"slow", Sleep: 200 * time.Millisecond})
Yeay for losing sleeps.

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go
File cmd/juju/plugin_test.go (right):

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go#newcode96
cmd/juju/plugin_test.go:96: // Thus if we don't start them in parallel,
we would deadlock
I didn't understand this comment till I went back and read the mp
description properly. But now it seems obvious, so can't think of
clarifications.

Maybe:

// Make plugins with dependencies that will deadlock unless started in
parallel

https://codereview.appspot.com/10234047/

Revision history for this message

John A Meinel (jameinel) wrote on 2013-06-13:

#

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go
File cmd/juju/plugin_test.go (right):

https://codereview.appspot.com/10234047/diff/1/cmd/juju/plugin_test.go#newcode96
cmd/juju/plugin_test.go:96: // Thus if we don't start them in parallel,
we would deadlock
On 2013/06/13 10:53:48, gz wrote:
> I didn't understand this comment till I went back and read the mp
description
> properly. But now it seems obvious, so can't think of clarifications.

> Maybe:

> // Make plugins with dependencies that will deadlock unless started in
parallel

Updated.

https://codereview.appspot.com/10234047/

juju-core

Merge lp://staging/~jameinel/juju-core/timing-test into lp://staging/~juju/juju-core/trunk

Commit message

Description of the change

Preview Diff

Subscribers