Merge lp:~rogpeppe/gozk/better-error-messages into lp:gozk/zookeeper

[Roger Peppe]

zookeeper: make error messages more informative.

https://codereview.appspot.com/5835045/

[Roger Peppe (rogpeppe)]

Reviewers: mp+97613_code.launchpad.net,

Message:
Please take a look.

Description:

https://code.launchpad.net/~rogpeppe/gozk/better-error-messages/+merge/97613

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M close_test.go
   M retry_test.go
   M zk.go
   M zk_test.go

[Gustavo Niemeyer (niemeyer)]

Very nice. Just a few ideas.

https://codereview.appspot.com/5835045/diff/2001/zk.go
File zk.go (right):

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode120
zk.go:120: // HasErrorCode returns whether the error has the given
It's nice to have this. I was a bit concerned it might be boring to deal
with it.

Can we please call the function as IsError? For example:

   if zookeeper.IsError(err, zookeeper.ZNONODE) { ... }

Also, for the comment, I suggest something like this:

// IsError returns whether err is an *Error with the given
// error code.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode457
zk.go:457: func errClosing(op string) error {
Can we please call this closingError and put it next to zkError? They're
pretty related.

Also, closingError should take a path, as many of the places it's used
have it in context.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode584
zk.go:584: return nil, nil, nil, errClosing("childrenW")
The upper/lower casing is inconsistent. We have "ACL", "addauth",
"setACL", and "childrenW". I suggest we lowercase them all completely.
This is an error message, and more of a hint than something that has to
reflect the method name used exactly.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode925
zk.go:925: if err != nil && !HasErrorCode(err, ZNONODE) {
err != nil isn't necessary here.

https://codereview.appspot.com/5835045/diff/2001/zk_test.go
File zk_test.go (right):

https://codereview.appspot.com/5835045/diff/2001/zk_test.go#newcode1
zk_test.go:1: package zookeeper_test
The Error.Error method has a few different possibilities now as far as
building the error message goes. Would you mind to add a test case for
those? We don't want to find out that we broke that logic at some point
in actual usage.

https://codereview.appspot.com/5835045/diff/2001/zk_test.go#newcode161
zk_test.go:161: c.Assert(err, ErrorMatches, `zookeeper: get
"/non-existent": no node`)
Nice!

https://codereview.appspot.com/5835045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/5835045/

[lp:~rogpeppe/gozk/better-error-messages]

39. By Roger Peppe on 2012-03-15

changes for review

40. By Roger Peppe on 2012-03-15

go fmt

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/5835045/diff/2001/zk.go
File zk.go (right):

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode120
zk.go:120: // HasErrorCode returns whether the error has the given
On 2012/03/15 14:31:38, niemeyer wrote:
> It's nice to have this. I was a bit concerned it might be boring to
deal with
> it.

> Can we please call the function as IsError? For example:

> if zookeeper.IsError(err, zookeeper.ZNONODE) { ... }

> Also, for the comment, I suggest something like this:

> // IsError returns whether err is an *Error with the given
> // error code.

yeah, that's nicer. done.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode457
zk.go:457: func errClosing(op string) error {
On 2012/03/15 14:31:38, niemeyer wrote:
> Can we please call this closingError and put it next to zkError?
They're pretty
> related.

> Also, closingError should take a path, as many of the places it's used
have it
> in context.

Done.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode584
zk.go:584: return nil, nil, nil, errClosing("childrenW")
On 2012/03/15 14:31:38, niemeyer wrote:
> The upper/lower casing is inconsistent. We have "ACL", "addauth",
"setACL", and
> "childrenW". I suggest we lowercase them all completely. This is an
error
> message, and more of a hint than something that has to reflect the
method name
> used exactly.

yeah, i wondered about that. done.

https://codereview.appspot.com/5835045/diff/2001/zk.go#newcode925
zk.go:925: if err != nil && !HasErrorCode(err, ZNONODE) {
On 2012/03/15 14:31:38, niemeyer wrote:
> err != nil isn't necessary here.

it is necessary, i think, otherwise we'll return when err is nil.

https://codereview.appspot.com/5835045/diff/2001/zk_test.go
File zk_test.go (right):

https://codereview.appspot.com/5835045/diff/2001/zk_test.go#newcode1
zk_test.go:1: package zookeeper_test
On 2012/03/15 14:31:38, niemeyer wrote:
> The Error.Error method has a few different possibilities now as far as
building
> the error message goes. Would you mind to add a test case for those?
We don't
> want to find out that we broke that logic at some point in actual
usage.

Done.

https://codereview.appspot.com/5835045/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/5835045/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/5835045/

[Roger Peppe (rogpeppe)]

*** Submitted:

zookeeper: make error messages more informative.

R=niemeyer
CC=
https://codereview.appspot.com/5835045

https://codereview.appspot.com/5835045/

=== modified file 'close_test.go'
--- close_test.go	2012-02-28 09:13:50 +0000
+++ close_test.go	2012-03-15 15:14:18 +0000
@@ -130,7 +130,7 @@
 		}
 		p.close()
 		err = f(conn, "/closetest")
-		c.Check(err, Equals, zk.ZCLOSING)
+		c.Check(zk.IsError(err, zk.ZCLOSING), Equals, true, Commentf("%v", err))
 	}
 }
 
 
=== modified file 'retry_test.go'
--- retry_test.go	2012-02-27 17:15:50 +0000
+++ retry_test.go	2012-03-15 15:14:18 +0000
@@ -209,7 +209,7 @@
 			return "", nil
 		})
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNOAUTH)
+	c.Check(zk.IsError(err, zk.ZNOAUTH), Equals, true, Commentf("%v", err))
 
 	stat, err := conn.Exists("/test")
 	c.Assert(err, IsNil)
@@ -232,7 +232,7 @@
 			called = true
 			return "", nil
 		})
-	c.Assert(err, Equals, zk.ZNOAUTH)
+	c.Check(zk.IsError(err, zk.ZNOAUTH), Equals, true, Commentf("%v", err))
 
 	stat, err := conn.Exists("/test")
 	c.Assert(err, IsNil)
@@ -256,7 +256,7 @@
 			return "", nil
 		})
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNOAUTH)
+	c.Check(zk.IsError(err, zk.ZNOAUTH), Equals, true, Commentf("%v", err))
 
 	stat, err := conn.Exists("/test/sub")
 	c.Assert(err, IsNil)
 
=== modified file 'zk.go'
--- zk.go	2012-02-27 17:15:50 +0000
+++ zk.go	2012-03-15 15:14:18 +0000
@@ -98,57 +98,89 @@
 }
 
 // Error represents a ZooKeeper error.
-type Error int
+type Error struct {
+	Op   string
+	Code ErrorCode
+	// SystemError holds an error if Code is ZSYSTEMERROR.
+	SystemError error
+	Path        string
+}
+
+func (e *Error) Error() string {
+	s := e.Code.String()
+	if e.Code == ZSYSTEMERROR && e.SystemError != nil {
+		s = e.SystemError.Error()
+	}
+	if e.Path == "" {
+		return fmt.Sprintf("zookeeper: %s: %v", e.Op, s)
+	}
+	return fmt.Sprintf("zookeeper: %s %q: %v", e.Op, e.Path, s)
+}
+
+// IsError returns whether the error is a *Error
+// with the given error code.
+func IsError(err error, code ErrorCode) bool {
+	if err, _ := err.(*Error); err != nil {
+		return err.Code == code
+	}
+	return false
+}
+
+// ErrorCode represents a kind of ZooKeeper error.
+type ErrorCode int
 
 const (
-	ZOK                      Error = C.ZOK
-	ZSYSTEMERROR             Error = C.ZSYSTEMERROR
-	ZRUNTIMEINCONSISTENCY    Error = C.ZRUNTIMEINCONSISTENCY
-	ZDATAINCONSISTENCY       Error = C.ZDATAINCONSISTENCY
-	ZCONNECTIONLOSS          Error = C.ZCONNECTIONLOSS
-	ZMARSHALLINGERROR        Error = C.ZMARSHALLINGERROR
-	ZUNIMPLEMENTED           Error = C.ZUNIMPLEMENTED
-	ZOPERATIONTIMEOUT        Error = C.ZOPERATIONTIMEOUT
-	ZBADARGUMENTS            Error = C.ZBADARGUMENTS
-	ZINVALIDSTATE            Error = C.ZINVALIDSTATE
-	ZAPIERROR                Error = C.ZAPIERROR
-	ZNONODE                  Error = C.ZNONODE
-	ZNOAUTH                  Error = C.ZNOAUTH
-	ZBADVERSION              Error = C.ZBADVERSION
-	ZNOCHILDRENFOREPHEMERALS Error = C.ZNOCHILDRENFOREPHEMERALS
-	ZNODEEXISTS              Error = C.ZNODEEXISTS
-	ZNOTEMPTY                Error = C.ZNOTEMPTY
-	ZSESSIONEXPIRED          Error = C.ZSESSIONEXPIRED
-	ZINVALIDCALLBACK         Error = C.ZINVALIDCALLBACK
-	ZINVALIDACL              Error = C.ZINVALIDACL
-	ZAUTHFAILED              Error = C.ZAUTHFAILED
-	ZCLOSING                 Error = C.ZCLOSING
-	ZNOTHING                 Error = C.ZNOTHING
-	ZSESSIONMOVED            Error = C.ZSESSIONMOVED
+	ZOK                      ErrorCode = C.ZOK
+	ZSYSTEMERROR             ErrorCode = C.ZSYSTEMERROR
+	ZRUNTIMEINCONSISTENCY    ErrorCode = C.ZRUNTIMEINCONSISTENCY
+	ZDATAINCONSISTENCY       ErrorCode = C.ZDATAINCONSISTENCY
+	ZCONNECTIONLOSS          ErrorCode = C.ZCONNECTIONLOSS
+	ZMARSHALLINGERROR        ErrorCode = C.ZMARSHALLINGERROR
+	ZUNIMPLEMENTED           ErrorCode = C.ZUNIMPLEMENTED
+	ZOPERATIONTIMEOUT        ErrorCode = C.ZOPERATIONTIMEOUT
+	ZBADARGUMENTS            ErrorCode = C.ZBADARGUMENTS
+	ZINVALIDSTATE            ErrorCode = C.ZINVALIDSTATE
+	ZAPIERROR                ErrorCode = C.ZAPIERROR
+	ZNONODE                  ErrorCode = C.ZNONODE
+	ZNOAUTH                  ErrorCode = C.ZNOAUTH
+	ZBADVERSION              ErrorCode = C.ZBADVERSION
+	ZNOCHILDRENFOREPHEMERALS ErrorCode = C.ZNOCHILDRENFOREPHEMERALS
+	ZNODEEXISTS              ErrorCode = C.ZNODEEXISTS
+	ZNOTEMPTY                ErrorCode = C.ZNOTEMPTY
+	ZSESSIONEXPIRED          ErrorCode = C.ZSESSIONEXPIRED
+	ZINVALIDCALLBACK         ErrorCode = C.ZINVALIDCALLBACK
+	ZINVALIDACL              ErrorCode = C.ZINVALIDACL
+	ZAUTHFAILED              ErrorCode = C.ZAUTHFAILED
+	ZCLOSING                 ErrorCode = C.ZCLOSING
+	ZNOTHING                 ErrorCode = C.ZNOTHING
+	ZSESSIONMOVED            ErrorCode = C.ZSESSIONMOVED
 )
 
-func (error Error) Error() string {
-	return C.GoString(C.zerror(C.int(error))) // Static, no need to free it.
+func (code ErrorCode) String() string {
+	return C.GoString(C.zerror(C.int(code))) // Static, no need to free it.
 }
 
 // zkError creates an appropriate error return from
 // a ZooKeeper status and the errno return from a C API
 // call.
-func zkError(rc C.int, cerr error) error {
-	code := Error(rc)
-	switch code {
-	case ZOK:
+func zkError(rc C.int, cerr error, op, path string) error {
+	code := ErrorCode(rc)
+	if code == ZOK {
 		return nil
+	}
+	err := &Error{
+		Op:   op,
+		Code: code,
+		Path: path,
+	}
+	if code == ZSYSTEMERROR {
+		err.SystemError = cerr
+	}
+	return err
+}
 
-	case ZSYSTEMERROR:
-		// If a ZooKeeper call returns ZSYSTEMERROR, then
-		// errno becomes significant. If errno has not been
-		// set, then we will return ZSYSTEMERROR nonetheless.
-		if cerr != nil {
-			return cerr
-		}
-	}
-	return code
+func closingError(op, path string) error {
+	return zkError(C.int(ZCLOSING), nil, op, path)
 }
 
 // Constants for SetLogLevel.
@@ -419,7 +451,7 @@
 	C.free(unsafe.Pointer(cservers))
 	if handle == nil {
 		conn.closeAllWatches()
-		return nil, nil, zkError(C.int(ZSYSTEMERROR), cerr)
+		return nil, nil, zkError(C.int(ZSYSTEMERROR), cerr, "dial", "")
 	}
 	conn.handle = handle
 	runWatchLoop()
@@ -444,7 +476,7 @@
 	if conn.handle == nil {
 		// ZooKeeper may hang indefinitely if a handler is closed twice,
 		// so we get in the way and prevent it from happening.
-		return ZCLOSING
+		return closingError("close", "")
 	}
 	rc, cerr := C.zookeeper_close(conn.handle)
 
@@ -454,7 +486,7 @@
 	// At this point, nothing else should need conn.handle.
 	conn.handle = nil
 
-	return zkError(rc, cerr)
+	return zkError(rc, cerr, "close", "")
 }
 
 // Get returns the data and status from an existing node.  err will be nil,
@@ -464,7 +496,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return "", nil, ZCLOSING
+		return "", nil, closingError("get", path)
 	}
 
 	cpath := C.CString(path)
@@ -476,7 +508,7 @@
 	var cstat Stat
 	rc, cerr := C.zoo_wget(conn.handle, cpath, nil, nil, cbuffer, &cbufferLen, &cstat.c)
 	if rc != C.ZOK {
-		return "", nil, zkError(rc, cerr)
+		return "", nil, zkError(rc, cerr, "get", path)
 	}
 
 	result := C.GoStringN(cbuffer, cbufferLen)
@@ -491,7 +523,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return "", nil, nil, ZCLOSING
+		return "", nil, nil, closingError("getw", path)
 	}
 
 	cpath := C.CString(path)
@@ -506,7 +538,7 @@
 	rc, cerr := C.zoo_wget(conn.handle, cpath, C.watch_handler, unsafe.Pointer(watchId), cbuffer, &cbufferLen, &cstat.c)
 	if rc != C.ZOK {
 		conn.forgetWatch(watchId)
-		return "", nil, nil, zkError(rc, cerr)
+		return "", nil, nil, zkError(rc, cerr, "getw", path)
 	}
 
 	result := C.GoStringN(cbuffer, cbufferLen)
@@ -519,7 +551,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, nil, ZCLOSING
+		return nil, nil, closingError("children", path)
 	}
 
 	cpath := C.CString(path)
@@ -536,7 +568,7 @@
 	if rc == C.ZOK {
 		stat = &cstat
 	} else {
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "children", path)
 	}
 	return
 }
@@ -549,7 +581,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, nil, nil, ZCLOSING
+		return nil, nil, nil, closingError("childrenw", path)
 	}
 
 	cpath := C.CString(path)
@@ -570,7 +602,7 @@
 		watch = watchChannel
 	} else {
 		conn.forgetWatch(watchId)
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "childrenw", path)
 	}
 	return
 }
@@ -595,7 +627,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, ZCLOSING
+		return nil, closingError("exists", path)
 	}
 
 	cpath := C.CString(path)
@@ -610,7 +642,7 @@
 	if rc == C.ZOK {
 		stat = &cstat
 	} else if rc != C.ZNONODE {
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "exists", path)
 	}
 	return
 }
@@ -624,7 +656,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, nil, ZCLOSING
+		return nil, nil, closingError("existsw", path)
 	}
 
 	cpath := C.CString(path)
@@ -638,7 +670,7 @@
 	// We diverge a bit from the usual here: a ZNONODE is not an error
 	// for an exists call, otherwise every Exists call would have to check
 	// for err != nil and err.Code() != ZNONODE.
-	switch Error(rc) {
+	switch ErrorCode(rc) {
 	case ZOK:
 		stat = &cstat
 		watch = watchChannel
@@ -646,7 +678,7 @@
 		watch = watchChannel
 	default:
 		conn.forgetWatch(watchId)
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "existsw", path)
 	}
 	return
 }
@@ -664,7 +696,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return "", ZCLOSING
+		return "", closingError("close", path)
 	}
 
 	cpath := C.CString(path)
@@ -684,7 +716,7 @@
 	if rc == C.ZOK {
 		pathCreated = C.GoString(cpathCreated)
 	} else {
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "create", path)
 	}
 	return
 }
@@ -701,7 +733,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, ZCLOSING
+		return nil, closingError("set", path)
 	}
 
 	cpath := C.CString(path)
@@ -714,7 +746,7 @@
 	if rc == C.ZOK {
 		stat = &cstat
 	} else {
-		err = zkError(rc, cerr)
+		err = zkError(rc, cerr, "set", path)
 	}
 	return
 }
@@ -726,13 +758,13 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return ZCLOSING
+		return closingError("delete", path)
 	}
 
 	cpath := C.CString(path)
 	defer C.free(unsafe.Pointer(cpath))
 	rc, cerr := C.zoo_delete(conn.handle, cpath, C.int(version))
-	return zkError(rc, cerr)
+	return zkError(rc, cerr, "delete", path)
 }
 
 // AddAuth adds a new authentication certificate to the ZooKeeper
@@ -744,7 +776,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return ZCLOSING
+		return closingError("addauth", "")
 	}
 
 	cscheme := C.CString(scheme)
@@ -760,13 +792,13 @@
 
 	rc, cerr := C.zoo_add_auth(conn.handle, cscheme, ccert, C.int(len(cert)), C.handle_void_completion, unsafe.Pointer(data))
 	if rc != C.ZOK {
-		return zkError(rc, cerr)
+		return zkError(rc, cerr, "addauth", "")
 	}
 
 	C.wait_for_completion(data)
 
 	rc = C.int(uintptr(data.data))
-	return zkError(rc, nil)
+	return zkError(rc, nil, "addauth", "")
 }
 
 // ACL returns the access control list for path.
@@ -774,7 +806,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return nil, nil, ZCLOSING
+		return nil, nil, closingError("acl", path)
 	}
 
 	cpath := C.CString(path)
@@ -785,7 +817,7 @@
 	var cstat Stat
 	rc, cerr := C.zoo_get_acl(conn.handle, cpath, &caclv, &cstat.c)
 	if rc != C.ZOK {
-		return nil, nil, zkError(rc, cerr)
+		return nil, nil, zkError(rc, cerr, "acl", path)
 	}
 
 	aclv := parseACLVector(&caclv)
@@ -798,7 +830,7 @@
 	conn.mutex.RLock()
 	defer conn.mutex.RUnlock()
 	if conn.handle == nil {
-		return ZCLOSING
+		return closingError("setacl", path)
 	}
 
 	cpath := C.CString(path)
@@ -808,7 +840,7 @@
 	defer C.deallocate_ACL_vector(caclv)
 
 	rc, cerr := C.zoo_set_acl(conn.handle, cpath, C.int(version), caclv)
-	return zkError(rc, cerr)
+	return zkError(rc, cerr, "setacl", path)
 }
 
 func parseACLVector(caclv *C.struct_ACL_vector) []ACL {
@@ -890,7 +922,7 @@
 func (conn *Conn) RetryChange(path string, flags int, acl []ACL, changeFunc ChangeFunc) error {
 	for {
 		oldValue, oldStat, err := conn.Get(path)
-		if err != nil && err != ZNONODE {
+		if err != nil && !IsError(err, ZNONODE) {
 			return err
 		}
 		newValue, err := changeFunc(oldValue, oldStat)
@@ -899,7 +931,7 @@
 		}
 		if oldStat == nil {
 			_, err := conn.Create(path, newValue, flags, acl)
-			if err == nil || err != ZNODEEXISTS {
+			if err == nil || !IsError(err, ZNODEEXISTS) {
 				return err
 			}
 			continue
@@ -908,7 +940,7 @@
 			return nil // Nothing to do.
 		}
 		_, err = conn.Set(path, newValue, oldStat.Version())
-		if err == nil || (err != ZBADVERSION && err != ZNONODE) {
+		if err == nil || !IsError(err, ZBADVERSION) && !IsError(err, ZNONODE) {
 			return err
 		}
 	}
 
=== modified file 'zk_test.go'
--- zk_test.go	2012-02-27 17:15:50 +0000
+++ zk_test.go	2012-03-15 15:14:18 +0000
@@ -1,6 +1,7 @@
 package zookeeper_test
 
 import (
+	"errors"
 	. "launchpad.net/gocheck"
 	zk "launchpad.net/gozk/zookeeper"
 	"time"
@@ -25,7 +26,45 @@
 	}
 	c.Assert(conn, IsNil)
 	c.Assert(watch, IsNil)
-	c.Assert(err, ErrorMatches, "invalid argument")
+	c.Assert(err, ErrorMatches, "zookeeper: dial: invalid argument")
+}
+
+func (s *S) TestErrorMessages(c *C) {
+	tests := []struct {
+		err zk.Error
+		msg string
+	}{{
+		zk.Error{
+			Op:   "foo",
+			Code: zk.ZNONODE,
+			Path: "/blah",
+		},
+		`zookeeper: foo "/blah": no node`,
+	}, {
+		zk.Error{
+			Op:   "foo",
+			Code: zk.ZNONODE,
+		},
+		`zookeeper: foo: no node`,
+	}, {
+		zk.Error{
+			Op:          "foo",
+			Code:        zk.ZSYSTEMERROR,
+			SystemError: errors.New("an error"),
+			Path:        "/blah",
+		},
+		`zookeeper: foo "/blah": an error`,
+	}, {
+		zk.Error{
+			Op:   "foo",
+			Code: zk.ZSYSTEMERROR,
+			Path: "/blah",
+		},
+		`zookeeper: foo "/blah": system error`,
+	}}
+	for _, t := range tests {
+		c.Check(t.err.Error(), Equals, t.msg)
+	}
 }
 
 func (s *S) TestRecvTimeoutInitParameter(c *C) {
@@ -42,7 +81,7 @@
 	for i := 0; i != 1000; i++ {
 		_, _, err := conn.Get("/zookeeper")
 		if err != nil {
-			c.Assert(err, ErrorMatches, "operation timeout")
+			c.Check(zk.IsError(err, zk.ZOPERATIONTIMEOUT), Equals, true, Commentf("%v", err))
 			c.SucceedNow()
 		}
 	}
@@ -158,8 +197,8 @@
 
 	c.Assert(data, Equals, "")
 	c.Assert(stat, IsNil)
-	c.Assert(err, ErrorMatches, "no node")
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Assert(err, ErrorMatches, `zookeeper: get "/non-existent": no node`)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 }
 
 func (s *S) TestCreateAndGet(c *C) {
@@ -171,7 +210,7 @@
 
 	// Check the error condition from Create().
 	_, err = conn.Create(path, "", zk.EPHEMERAL, zk.WorldACL(zk.PERM_ALL))
-	c.Assert(err, ErrorMatches, "node exists")
+	c.Check(zk.IsError(err, zk.ZNODEEXISTS), Equals, true, Commentf("%v", err))
 
 	data, _, err := conn.Get(path)
 	c.Assert(err, IsNil)
@@ -270,7 +309,7 @@
 
 	_, _, watch, err := conn.GetW("/test")
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 	c.Assert(watch, IsNil)
 
 	c.Check(zk.CountPendingWatches(), Equals, 1)
@@ -304,7 +343,7 @@
 	c.Assert(err, IsNil)
 	err = conn.Close()
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZCLOSING)
+	c.Check(zk.IsError(err, zk.ZCLOSING), Equals, true, Commentf("%v", err))
 }
 
 func (s *S) TestChildren(c *C) {
@@ -316,7 +355,7 @@
 	c.Assert(stat.NumChildren(), Equals, 1)
 
 	children, stat, err = conn.Children("/non-existent")
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 	c.Assert(children, IsNil)
 	c.Assert(stat, IsNil)
 }
@@ -383,7 +422,7 @@
 
 	_, stat, watch, err := conn.ChildrenW("/test")
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 	c.Assert(watch, IsNil)
 	c.Assert(stat, IsNil)
 
@@ -447,7 +486,7 @@
 
 	stat, watch, err := conn.ExistsW("///")
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZBADARGUMENTS)
+	c.Check(zk.IsError(err, zk.ZBADARGUMENTS), Equals, true, Commentf("%v", err))
 	c.Assert(stat, IsNil)
 	c.Assert(watch, IsNil)
 
@@ -462,14 +501,14 @@
 
 	err = conn.Delete("/test", 5)
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZBADVERSION)
+	c.Check(zk.IsError(err, zk.ZBADVERSION), Equals, true, Commentf("%v", err))
 
 	err = conn.Delete("/test", -1)
 	c.Assert(err, IsNil)
 
 	err = conn.Delete("/test", -1)
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 }
 
 func (s *S) TestClientIdAndReInit(c *C) {
@@ -518,7 +557,7 @@
 
 	acl, stat, err = conn.ACL("/non-existent")
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNONODE)
+	c.Check(zk.IsError(err, zk.ZNONODE), Equals, true, Commentf("%v", err))
 	c.Assert(acl, IsNil)
 	c.Assert(stat, IsNil)
 }
@@ -531,7 +570,7 @@
 
 	err = conn.SetACL("/test", zk.WorldACL(zk.PERM_ALL), 5)
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZBADVERSION)
+	c.Check(zk.IsError(err, zk.ZBADVERSION), Equals, true, Commentf("%v", err))
 
 	err = conn.SetACL("/test", zk.WorldACL(zk.PERM_READ), -1)
 	c.Assert(err, IsNil)
@@ -551,7 +590,7 @@
 
 	_, _, err = conn.Get("/test")
 	c.Assert(err, NotNil)
-	c.Assert(err, Equals, zk.ZNOAUTH)
+	c.Check(zk.IsError(err, zk.ZNOAUTH), Equals, true, Commentf("%v", err))
 
 	err = conn.AddAuth("digest", "joe:passwd")
 	c.Assert(err, IsNil)