Merge lp:~rogpeppe/juju-core/559-fix-err-deepequals into lp:~go-bot/juju-core/trunk

[Roger Peppe]

various: fix error DeepEquals comparisons

We should not be comparing errors with DeepEquals
in general - they have private fields, and this will
break when using adding trace information with errgo.

https://codereview.appspot.com/89660043/

various: fix error DeepEquals comparisons

We should not be comparing errors with DeepEquals
in general - they have private fields, and this will
break when using adding trace information with errgo.

https://codereview.appspot.com/89660043/

[Roger Peppe (rogpeppe)]

Reviewers: mp+216540_code.launchpad.net,

Message:
Please take a look.

Description:
various: fix error DeepEquals comparisons

We should not be comparing errors with DeepEquals
in general - they have private fields, and this will
break when using adding trace information with errgo.

https://code.launchpad.net/~rogpeppe/juju-core/559-fix-err-deepequals/+merge/216540

(do not edit description out of merge proposal)

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

Affected files (+16, -12 lines):
   A [revision details]
   M state/machine_test.go
   M testing/locking_test.go
   M utils/apt_test.go
   M worker/instancepoller/aggregate_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: tarmac-20140418183250-hzz3f8zpw6ift0az
+New revision: <email address hidden>

Index: state/machine_test.go
=== modified file 'state/machine_test.go'
--- state/machine_test.go 2014-04-18 16:37:28 +0000
+++ state/machine_test.go 2014-04-20 12:41:40 +0000
@@ -137,7 +137,8 @@
   c.Assert(err, gc.FitsTypeOf, &state.HasContainersError{})
   c.Assert(err, gc.ErrorMatches, `machine 1 is hosting
containers "1/lxc/0"`)
   err1 := s.machine.EnsureDead()
- c.Assert(err1, gc.DeepEquals, err)
+ c.Assert(err1, gc.FitsTypeOf, &state.HasContainersError{})
+ c.Assert(err1, gc.ErrorMatches, `machine 1 is hosting
containers "1/lxc/0"`)
   c.Assert(s.machine.Life(), gc.Equals, state.Alive)
  }

@@ -152,7 +153,8 @@
   c.Assert(err, gc.FitsTypeOf, &state.HasAssignedUnitsError{})
   c.Assert(err, gc.ErrorMatches, `machine 1 has unit "wordpress/0"
assigned`)
   err1 := s.machine.EnsureDead()
- c.Assert(err1, gc.DeepEquals, err)
+ c.Assert(err, gc.FitsTypeOf, &state.HasAssignedUnitsError{})
+ c.Assert(err, gc.ErrorMatches, `machine 1 has unit "wordpress/0"
assigned`)
   c.Assert(s.machine.Life(), gc.Equals, state.Alive)

   // Once no unit is assigned, lifecycle can advance.

Index: testing/locking_test.go
=== modified file 'testing/locking_test.go'
--- testing/locking_test.go 2013-08-19 11:20:02 +0000
+++ testing/locking_test.go 2014-04-20 12:40:45 +0000
@@ -4,7 +4,6 @@
  package testing

  import (
- "errors"
   "sync"

   gc "launchpad.net/gocheck"
@@ -29,8 +28,9 @@
   function := func() {}
   c.Check(
    func() { TestLockingFunction(&lock, function) },
- gc.Panics,
- errors.New("function did not obey lock"))
+ gc.PanicMatches,
+ "function did not obey lock",
+ )
  }

  func (LockingSuite) TestTestLockingFunctionDetectsFailureToReleaseLock(c
*gc.C) {
@@ -41,6 +41,7 @@
   }
   c.Check(
    func() { TestLockingFunction(&lock, function) },
- gc.Panics,
- errors.New("function did not release lock"))
+ gc.PanicMatches,
+ "function did not release lock",
+ )
  }

Index: utils/apt_test.go
=== modified file 'utils/apt_test.go'
--- utils/apt_test.go 2014-03-21 05:07:45 +0000
+++ utils/apt_test.go 2014-04-20 12:40:45 +0000
@@ -42,10 +42,9 @@
  func (s *AptSuite) TestAptGetError(c *gc.C) {
   const expected = `E: frobnicator failure detected`
   cmdError := fmt.Errorf("error")
-...

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/89660043/

[Go Bot (go-bot)]

The attempt to merge lp:~rogpeppe/juju-core/559-fix-err-deepequals into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.721s
ok launchpad.net/juju-core/agent/mongo 1.305s
ok launchpad.net/juju-core/agent/tools 0.225s
ok launchpad.net/juju-core/bzr 5.261s
ok launchpad.net/juju-core/cert 2.477s
ok launchpad.net/juju-core/charm 0.438s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.030s
ok launchpad.net/juju-core/cloudinit/sshinit 0.741s
ok launchpad.net/juju-core/cmd 0.172s
ok launchpad.net/juju-core/cmd/charm-admin 0.744s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/envcmd 0.170s
ok launchpad.net/juju-core/cmd/juju 228.307s
ok launchpad.net/juju-core/cmd/jujud 76.126s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.074s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.180s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.023s
ok launchpad.net/juju-core/container 0.039s
ok launchpad.net/juju-core/container/factory 0.046s
ok launchpad.net/juju-core/container/kvm 0.221s
ok launchpad.net/juju-core/container/kvm/mock 0.035s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.311s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.231s
ok launchpad.net/juju-core/environs 2.341s
ok launchpad.net/juju-core/environs/bootstrap 12.289s
ok launchpad.net/juju-core/environs/cloudinit 0.423s
ok launchpad.net/juju-core/environs/config 3.763s
ok launchpad.net/juju-core/environs/configstore 0.029s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.646s
ok launchpad.net/juju-core/environs/imagemetadata 0.444s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.040s
ok launchpad.net/juju-core/environs/jujutest 0.163s
ok launchpad.net/juju-core/environs/manual 11.746s
? launchpad.net/juju-core/environs/network [no test files]
ok launchpad.net/juju-core/environs/simplestreams 0.278s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.892s
ok launchpad.net/juju-core/environs/storage 0.923s
ok launchpad.net/juju-core/environs/sync 51.290s
ok launchpad.net/juju-core/environs/testing 0.142s
ok launchpad.net/juju-core/environs/tools 4.655s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.018s
ok launchpad.net/juju-core/instance 0.018s
? launchpad.net/juju-core/instance/...

[John A Meinel (jameinel)]

It looks like you hit a flakiness panic, but also an actual error:
# launchpad.net/juju-core/state_test
state/machine_test.go:155: err1 declared and not used

John
=:->

On Mon, Apr 21, 2014 at 5:15 PM, Go Bot <email address hidden> wrote:

> The proposal to merge lp:~rogpeppe/juju-core/559-fix-err-deepequals into
> lp:juju-core has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
>
> https://code.launchpad.net/~rogpeppe/juju-core/559-fix-err-deepequals/+merge/216540
> --
>
> https://code.launchpad.net/~rogpeppe/juju-core/559-fix-err-deepequals/+merge/216540
> You are subscribed to branch lp:juju-core.
>

Unmerged revisions

2654. By Roger Peppe on 2014-04-20

merge trunk

2653. By Roger Peppe on 2014-04-20

various: fix error deepequal comparisons in tests

=== modified file 'state/machine_test.go'
--- state/machine_test.go	2014-04-18 16:37:28 +0000
+++ state/machine_test.go	2014-04-20 12:44:03 +0000
@@ -137,7 +137,8 @@
 	c.Assert(err, gc.FitsTypeOf, &state.HasContainersError{})
 	c.Assert(err, gc.ErrorMatches, `machine 1 is hosting containers "1/lxc/0"`)
 	err1 := s.machine.EnsureDead()
-	c.Assert(err1, gc.DeepEquals, err)
+	c.Assert(err1, gc.FitsTypeOf, &state.HasContainersError{})
+	c.Assert(err1, gc.ErrorMatches, `machine 1 is hosting containers "1/lxc/0"`)
 	c.Assert(s.machine.Life(), gc.Equals, state.Alive)
 }
 
@@ -152,7 +153,8 @@
 	c.Assert(err, gc.FitsTypeOf, &state.HasAssignedUnitsError{})
 	c.Assert(err, gc.ErrorMatches, `machine 1 has unit "wordpress/0" assigned`)
 	err1 := s.machine.EnsureDead()
-	c.Assert(err1, gc.DeepEquals, err)
+	c.Assert(err, gc.FitsTypeOf, &state.HasAssignedUnitsError{})
+	c.Assert(err, gc.ErrorMatches, `machine 1 has unit "wordpress/0" assigned`)
 	c.Assert(s.machine.Life(), gc.Equals, state.Alive)
 
 	// Once no unit is assigned, lifecycle can advance.

=== modified file 'testing/locking_test.go'
--- testing/locking_test.go	2013-08-19 11:20:02 +0000
+++ testing/locking_test.go	2014-04-20 12:44:03 +0000
@@ -4,7 +4,6 @@
 package testing
 
 import (
-	"errors"
 	"sync"
 
 	gc "launchpad.net/gocheck"
@@ -29,8 +28,9 @@
 	function := func() {}
 	c.Check(
 		func() { TestLockingFunction(&lock, function) },
-		gc.Panics,
-		errors.New("function did not obey lock"))
+		gc.PanicMatches,
+		"function did not obey lock",
+	)
 }
 
 func (LockingSuite) TestTestLockingFunctionDetectsFailureToReleaseLock(c *gc.C) {
@@ -41,6 +41,7 @@
 	}
 	c.Check(
 		func() { TestLockingFunction(&lock, function) },
-		gc.Panics,
-		errors.New("function did not release lock"))
+		gc.PanicMatches,
+		"function did not release lock",
+	)
 }

=== modified file 'utils/apt_test.go'
--- utils/apt_test.go	2014-03-21 05:07:45 +0000
+++ utils/apt_test.go	2014-04-20 12:44:03 +0000
@@ -42,10 +42,9 @@
 func (s *AptSuite) TestAptGetError(c *gc.C) {
 	const expected = `E: frobnicator failure detected`
 	cmdError := fmt.Errorf("error")
-	cmdExpectedError := fmt.Errorf("apt-get failed: error")
 	cmdChan := s.HookCommandOutput(&utils.AptCommandOutput, []byte(expected), cmdError)
 	err := utils.AptGetInstall("foo")
-	c.Assert(err, gc.DeepEquals, cmdExpectedError)
+	c.Assert(err, gc.ErrorMatches, "apt-get failed: error")
 	cmd := <-cmdChan
 	c.Assert(cmd.Args, gc.DeepEquals, []string{
 		"apt-get", "--option=Dpkg::Options::=--force-confold",
@@ -172,10 +171,9 @@
 func (s *AptSuite) TestConfigProxyError(c *gc.C) {
 	const expected = `E: frobnicator failure detected`
 	cmdError := fmt.Errorf("error")
-	cmdExpectedError := fmt.Errorf("apt-config failed: error")
 	cmdChan := s.HookCommandOutput(&utils.AptCommandOutput, []byte(expected), cmdError)
 	out, err := utils.AptConfigProxy()
-	c.Assert(err, gc.DeepEquals, cmdExpectedError)
+	c.Assert(err, gc.ErrorMatches, "apt-config failed: error")
 	cmd := <-cmdChan
 	c.Assert(cmd.Args, gc.DeepEquals, []string{
 		"apt-config", "dump", "Acquire::http::Proxy",

=== modified file 'worker/instancepoller/aggregate_test.go'
--- worker/instancepoller/aggregate_test.go	2014-04-15 18:51:24 +0000
+++ worker/instancepoller/aggregate_test.go	2014-04-20 12:44:03 +0000
@@ -163,7 +163,8 @@
 	aggregator := newAggregator(testGetter)
 	_, err := aggregator.instanceInfo("foo")
 
-	c.Assert(err, gc.DeepEquals, errors.NotFoundf("instance foo"))
+	c.Assert(err, jc.Satisfies, errors.IsNotFound)
+	c.Assert(err, gc.ErrorMatches, "instance foo not found")
 }
 
 func (s *aggregateSuite) TestAddressesError(c *gc.C) {