Merge lp:~thumper/juju-core/moar-checkers into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Some useful checkers.

Also some testing functions used later, with their own test.

https://codereview.appspot.com/10395044/

Some useful checkers.

Also some testing functions used later, with their own test.

https://codereview.appspot.com/10395044/

[Tim Penhey (thumper)]

Reviewers: mp+170227_code.launchpad.net,

Message:
Please take a look.

Description:
Some useful checkers.

Also some testing functions used later, with their own test.

https://code.launchpad.net/~thumper/juju-core/moar-checkers/+merge/170227

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A testing/checkers/bool.go
   A testing/checkers/bool_test.go
   A testing/checkers/checker_test.go
   A testing/checkers/relop.go
   A testing/checkers/relop_test.go
   A testing/file.go
   A testing/file_test.go

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10395044/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/10395044/diff/3001/testing/checkers/bool.go
File testing/checkers/bool.go (right):

https://codereview.appspot.com/10395044/diff/3001/testing/checkers/bool.go#newcode27
testing/checkers/bool.go:27: return false, "obtained value not a bool"
If possible, also add the value and type to this error message, like for
the other greaterThan checker

https://codereview.appspot.com/10395044/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10395044/diff/3001/testing/checkers/bool.go
File testing/checkers/bool.go (right):

https://codereview.appspot.com/10395044/diff/3001/testing/checkers/bool.go#newcode27
testing/checkers/bool.go:27: return false, "obtained value not a bool"
On 2013/06/19 00:50:23, wallyworld wrote:
> If possible, also add the value and type to this error message, like
for the
> other greaterThan checker

Done.

https://codereview.appspot.com/10395044/

[John A Meinel (jameinel)]

LGTM

I have some comments about it, but the only one I really want to see
fixed is s/recieved/received/.

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool_test.go
File testing/checkers/bool_test.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool_test.go#newcode22
testing/checkers/bool_test.go:22: }
'received' which I think means you need to update the checker as well.

Would it be better to have the message:

expected "true" (type bool), received string: "foo"

Or to make the two halves consistent:

`expected type bool "true", received string "foo"`

I don't think you need the colon and to quote the string. Though I'm
soft on that.

Mostly, I'd like that if we are going to explicitly list what we
obtained, we should explicitly list what we expected.

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go
File testing/checkers/relop_test.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go#newcode24
testing/checkers/relop_test.go:24: }
I wonder if spelling this GreaterThan is best. Mostly because I'm
thinking of the permutations:
GreaterThanOrEqualTo

Which is pretty commonly abbreviated GTE.

I'm also hoping the infrastructure can give us a nice "Between" (though
again you have to watch out for whether one or both ends are open or
closed a < x < b vs a <= x < b, etc)

https://codereview.appspot.com/10395044/diff/7001/testing/file.go
File testing/file.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/file.go#newcode17
testing/file.go:17: }
Couldn't this be done as a checker too?

c.Assert(filename, IsNonEmptyFile) ?

I'm fine with this helper, but checkers do tend to be slightly more
consistent with how we do assertions.

Also, it lets you use "c.Check" when appropriate, rather than always
Assert (fail the test but apply more checks vs fail immediately and
stop).

https://codereview.appspot.com/10395044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool_test.go
File testing/checkers/bool_test.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool_test.go#newcode22
testing/checkers/bool_test.go:22: }
On 2013/06/19 04:32:28, jameinel wrote:
> 'received' which I think means you need to update the checker as well.

> Would it be better to have the message:

> expected "true" (type bool), received string: "foo"

> Or to make the two halves consistent:

> `expected type bool "true", received string "foo"`

> I don't think you need the colon and to quote the string. Though I'm
soft on
> that.

> Mostly, I'd like that if we are going to explicitly list what we
obtained, we
> should explicitly list what we expected.

The quotes are part of %#v, so if we had

   42, that would become:
     int:42

Fixed received spelling, went for:

   `expected bool:true, received string:"foo"`
and added a test
   `expected bool:true, received int:42`

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go
File testing/checkers/relop_test.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go#newcode24
testing/checkers/relop_test.go:24: }
On 2013/06/19 04:32:28, jameinel wrote:
> I wonder if spelling this GreaterThan is best. Mostly because I'm
thinking of
> the permutations:
> GreaterThanOrEqualTo

> Which is pretty commonly abbreviated GTE.

> I'm also hoping the infrastructure can give us a nice "Between"
(though again
> you have to watch out for whether one or both ends are open or closed
a < x < b
> vs a <= x < b, etc)

python testtools uses GreaterThan too :)

Yes, you can do a between, and I do have one for time.

https://codereview.appspot.com/10395044/diff/7001/testing/file.go
File testing/file.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/file.go#newcode17
testing/file.go:17: }
On 2013/06/19 04:32:28, jameinel wrote:
> Couldn't this be done as a checker too?

> c.Assert(filename, IsNonEmptyFile) ?

> I'm fine with this helper, but checkers do tend to be slightly more
consistent
> with how we do assertions.

> Also, it lets you use "c.Check" when appropriate, rather than always
Assert
> (fail the test but apply more checks vs fail immediately and stop).

Yes... although I had trouble thinking of it at first.

Will fix.

https://codereview.appspot.com/10395044/

[Frank Mueller (themue)]

LGTM, thanks for moving it into an own CL.

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool.go
File testing/checkers/bool.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool.go#newcode13
testing/checkers/bool.go:13: // IsTrue checker
Not needed, for the other checkers too. But please add godoc comments
for the exported symbols like IsTrue or IsFalse.

https://codereview.appspot.com/10395044/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool.go
File testing/checkers/bool.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/bool.go#newcode34
testing/checkers/bool.go:34: return false, fmt.Sprintf("expected type
bool, recieved %s:%#v", value.Kind(), params[0])
return false, fmt.Sprintf("expected type bool, received %s",
value.Type())

?
(gocheck itself will print the value for you - no need to print it
again).

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop.go
File testing/checkers/relop.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop.go#newcode51
testing/checkers/relop.go:51: return false, fmt.Sprintf("obtained value
%s:%#v not supported", p0value.Kind(), params[0])
fmt.Sprintf("expected numeric type, received %T", p0value)

?

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go
File testing/checkers/relop_test.go (right):

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go#newcode4
testing/checkers/relop_test.go:4: package checkers_test
i'm not sure we gain much by having a separate test file for each kind
of checker. i'd put them all in one file and test suite
(checkers_test.go), making it easy to scan 'em all at once.

https://codereview.appspot.com/10395044/diff/7001/testing/checkers/relop_test.go#newcode8
testing/checkers/relop_test.go:8: .
"launchpad.net/juju-core/testing/checkers"
I object to the spreading use of import to .

gocheck itself is bad enough.

Rather than polluting the local package name space,
could we choose a suitable short package identifier
or alias please? Or just use "checkers" itself.

if we adopted the fairly standard abbreviations
that john suggests, we could have something like:

c.Assert(45, checkers.GT, 42)

which isn't too much typing, and it's obvious where
the names come from.

https://codereview.appspot.com/10395044/

=== added file 'testing/checkers/bool.go'
--- testing/checkers/bool.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/bool.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,35 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers
+
+import (
+	"fmt"
+	"reflect"
+
+	. "launchpad.net/gocheck"
+)
+
+// IsTrue checker
+
+type isTrueChecker struct {
+	*CheckerInfo
+}
+
+var IsTrue Checker = &isTrueChecker{
+	&CheckerInfo{Name: "IsTrue", Params: []string{"obtained"}},
+}
+
+var IsFalse Checker = Not(IsTrue)
+
+func (checker *isTrueChecker) Check(params []interface{}, names []string) (result bool, error string) {
+
+	value := reflect.ValueOf(params[0])
+
+	switch value.Kind() {
+	case reflect.Bool:
+		return value.Bool(), ""
+	}
+
+	return false, fmt.Sprintf("expected bool:true, received %s:%#v", value.Kind(), params[0])
+}

=== added file 'testing/checkers/bool_test.go'
--- testing/checkers/bool_test.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/bool_test.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,35 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers_test
+
+import (
+	. "launchpad.net/gocheck"
+	. "launchpad.net/juju-core/testing/checkers"
+)
+
+type BoolSuite struct{}
+
+var _ = Suite(&BoolSuite{})
+
+func (s *BoolSuite) TestIsTrue(c *C) {
+	c.Assert(true, IsTrue)
+	c.Assert(false, Not(IsTrue))
+
+	result, msg := IsTrue.Check([]interface{}{false}, nil)
+	c.Assert(result, Equals, false)
+	c.Assert(msg, Equals, "")
+
+	result, msg = IsTrue.Check([]interface{}{"foo"}, nil)
+	c.Assert(result, Equals, false)
+	c.Check(msg, Equals, `expected bool:true, received string:"foo"`)
+
+	result, msg = IsTrue.Check([]interface{}{42}, nil)
+	c.Assert(result, Equals, false)
+	c.Assert(msg, Equals, `expected bool:true, received int:42`)
+}
+
+func (s *BoolSuite) TestIsFalse(c *C) {
+	c.Assert(false, IsFalse)
+	c.Assert(true, Not(IsFalse))
+}

=== added file 'testing/checkers/checker_test.go'
--- testing/checkers/checker_test.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/checker_test.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,27 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers_test
+
+import (
+	"testing"
+
+	. "launchpad.net/gocheck"
+	. "launchpad.net/juju-core/testing/checkers"
+)
+
+func Test(t *testing.T) { TestingT(t) }
+
+type CheckerSuite struct{}
+
+var _ = Suite(&CheckerSuite{})
+
+func (s *CheckerSuite) TestHasPrefix(c *C) {
+	c.Assert("foo bar", HasPrefix, "foo")
+	c.Assert("foo bar", Not(HasPrefix), "omg")
+}
+
+func (s *CheckerSuite) TestHasSuffix(c *C) {
+	c.Assert("foo bar", HasSuffix, "bar")
+	c.Assert("foo bar", Not(HasSuffix), "omg")
+}

=== added file 'testing/checkers/file.go'
--- testing/checkers/file.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/file.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,98 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers
+
+import (
+	"fmt"
+	"os"
+	"reflect"
+
+	. "launchpad.net/gocheck"
+)
+
+// IsNonEmptyFile checker
+
+type isNonEmptyFileChecker struct {
+	*CheckerInfo
+}
+
+var IsNonEmptyFile Checker = &isNonEmptyFileChecker{
+	&CheckerInfo{Name: "IsNonEmptyFile", Params: []string{"obtained"}},
+}
+
+func (checker *isNonEmptyFileChecker) Check(params []interface{}, names []string) (result bool, error string) {
+	filename, isString := stringOrStringer(params[0])
+	if isString {
+		fileInfo, err := os.Stat(filename)
+		if os.IsNotExist(err) {
+			return false, fmt.Sprintf("%s does not exist", filename)
+		} else if err != nil {
+			return false, fmt.Sprintf("other stat error: %v", err)
+		}
+		if fileInfo.Size() > 0 {
+			return true, ""
+		} else {
+			return false, fmt.Sprintf("%s is empty", filename)
+		}
+	}
+
+	value := reflect.ValueOf(params[0])
+	return false, fmt.Sprintf("obtained value is not a string and has no .String(), %s:%#v", value.Kind(), params[0])
+}
+
+// IsDirectory checker
+
+type isDirectoryChecker struct {
+	*CheckerInfo
+}
+
+var IsDirectory Checker = &isDirectoryChecker{
+	&CheckerInfo{Name: "IsDirectory", Params: []string{"obtained"}},
+}
+
+func (checker *isDirectoryChecker) Check(params []interface{}, names []string) (result bool, error string) {
+	path, isString := stringOrStringer(params[0])
+	if isString {
+		fileInfo, err := os.Stat(path)
+		if os.IsNotExist(err) {
+			return false, fmt.Sprintf("%s does not exist", path)
+		} else if err != nil {
+			return false, fmt.Sprintf("other stat error: %v", err)
+		}
+		if fileInfo.IsDir() {
+			return true, ""
+		} else {
+			return false, fmt.Sprintf("%s is not a directory", path)
+		}
+	}
+
+	value := reflect.ValueOf(params[0])
+	return false, fmt.Sprintf("obtained value is not a string and has no .String(), %s:%#v", value.Kind(), params[0])
+}
+
+// DoesNotExist checker makes sure the path specified doesn't exist.
+
+type doesNotExistChecker struct {
+	*CheckerInfo
+}
+
+var DoesNotExist Checker = &doesNotExistChecker{
+	&CheckerInfo{Name: "DoesNotExist", Params: []string{"obtained"}},
+}
+
+func (checker *doesNotExistChecker) Check(params []interface{}, names []string) (result bool, error string) {
+	path, isString := stringOrStringer(params[0])
+	if isString {
+		_, err := os.Stat(path)
+		if os.IsNotExist(err) {
+			return true, ""
+		} else if err != nil {
+			return false, fmt.Sprintf("other stat error: %v", err)
+		}
+		return false, fmt.Sprintf("%s exists", path)
+	}
+
+	value := reflect.ValueOf(params[0])
+	return false, fmt.Sprintf("obtained value is not a string and has no .String(), %s:%#v", value.Kind(), params[0])
+}

=== added file 'testing/checkers/file_test.go'
--- testing/checkers/file_test.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/file_test.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,97 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers_test
+
+import (
+	"fmt"
+	"io/ioutil"
+	"path/filepath"
+
+	. "launchpad.net/gocheck"
+	. "launchpad.net/juju-core/testing/checkers"
+)
+
+type FileSuite struct{}
+
+var _ = Suite(&FileSuite{})
+
+func (s *FileSuite) TestIsNonEmptyFile(c *C) {
+	file, err := ioutil.TempFile(c.MkDir(), "")
+	c.Assert(err, IsNil)
+	fmt.Fprintf(file, "something")
+	file.Close()
+
+	c.Assert(file.Name(), IsNonEmptyFile)
+}
+
+func (s *FileSuite) TestIsNonEmptyFileWithEmptyFile(c *C) {
+	file, err := ioutil.TempFile(c.MkDir(), "")
+	c.Assert(err, IsNil)
+	file.Close()
+
+	result, message := IsNonEmptyFile.Check([]interface{}{file.Name()}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, file.Name()+" is empty")
+}
+
+func (s *FileSuite) TestIsNonEmptyFileWithMissingFile(c *C) {
+	name := filepath.Join(c.MkDir(), "missing")
+
+	result, message := IsNonEmptyFile.Check([]interface{}{name}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, name+" does not exist")
+}
+
+func (s *FileSuite) TestIsNonEmptyFileWithNumber(c *C) {
+	result, message := IsNonEmptyFile.Check([]interface{}{42}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, "obtained value is not a string and has no .String(), int:42")
+}
+
+func (s *FileSuite) TestIsDirectory(c *C) {
+	dir := c.MkDir()
+	c.Assert(dir, IsDirectory)
+}
+
+func (s *FileSuite) TestIsDirectoryMissing(c *C) {
+	absentDir := filepath.Join(c.MkDir(), "foo")
+
+	result, message := IsDirectory.Check([]interface{}{absentDir}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, absentDir+" does not exist")
+}
+
+func (s *FileSuite) TestIsDirectoryWithFile(c *C) {
+	file, err := ioutil.TempFile(c.MkDir(), "")
+	c.Assert(err, IsNil)
+	file.Close()
+
+	result, message := IsDirectory.Check([]interface{}{file.Name()}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, file.Name()+" is not a directory")
+}
+
+func (s *FileSuite) TestIsDirectoryWithNumber(c *C) {
+	result, message := IsDirectory.Check([]interface{}{42}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, "obtained value is not a string and has no .String(), int:42")
+}
+
+func (s *FileSuite) TestDoesNotExist(c *C) {
+	absentDir := filepath.Join(c.MkDir(), "foo")
+	c.Assert(absentDir, DoesNotExist)
+}
+
+func (s *FileSuite) TestDoesNotExistWithPath(c *C) {
+	dir := c.MkDir()
+	result, message := DoesNotExist.Check([]interface{}{dir}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, dir+" exists")
+}
+
+func (s *FileSuite) TestDoesNotExistWithNumber(c *C) {
+	result, message := DoesNotExist.Check([]interface{}{42}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(message, Equals, "obtained value is not a string and has no .String(), int:42")
+}

=== added file 'testing/checkers/relop.go'
--- testing/checkers/relop.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/relop.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,52 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers
+
+import (
+	"fmt"
+	"reflect"
+
+	. "launchpad.net/gocheck"
+)
+
+// GreaterThan checker
+
+type greaterThanChecker struct {
+	*CheckerInfo
+}
+
+var GreaterThan Checker = &greaterThanChecker{
+	&CheckerInfo{Name: "GreaterThan", Params: []string{"obtained", "expected"}},
+}
+
+func (checker *greaterThanChecker) Check(params []interface{}, names []string) (result bool, error string) {
+	defer func() {
+		if v := recover(); v != nil {
+			result = false
+			error = fmt.Sprint(v)
+		}
+	}()
+
+	p0value := reflect.ValueOf(params[0])
+	p1value := reflect.ValueOf(params[1])
+	switch p0value.Kind() {
+	case reflect.Int,
+		reflect.Int8,
+		reflect.Int16,
+		reflect.Int32,
+		reflect.Int64:
+		return p0value.Int() > p1value.Int(), ""
+	case reflect.Uint,
+		reflect.Uint8,
+		reflect.Uint16,
+		reflect.Uint32,
+		reflect.Uint64:
+		return p0value.Uint() > p1value.Uint(), ""
+	case reflect.Float32,
+		reflect.Float64:
+		return p0value.Float() > p1value.Float(), ""
+	default:
+	}
+	return false, fmt.Sprintf("obtained value %s:%#v not supported", p0value.Kind(), params[0])
+}

=== added file 'testing/checkers/relop_test.go'
--- testing/checkers/relop_test.go	1970-01-01 00:00:00 +0000
+++ testing/checkers/relop_test.go	2013-06-19 05:41:26 +0000
@@ -0,0 +1,24 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package checkers_test
+
+import (
+	. "launchpad.net/gocheck"
+	. "launchpad.net/juju-core/testing/checkers"
+)
+
+type RelopSuite struct{}
+
+var _ = Suite(&RelopSuite{})
+
+func (s *RelopSuite) TestGreaterThan(c *C) {
+	c.Assert(45, GreaterThan, 42)
+	c.Assert(2.25, GreaterThan, 1.0)
+	c.Assert(42, Not(GreaterThan), 42)
+	c.Assert(10, Not(GreaterThan), 42)
+
+	result, msg := GreaterThan.Check([]interface{}{"Hello", "World"}, nil)
+	c.Assert(result, IsFalse)
+	c.Assert(msg, Equals, `obtained value string:"Hello" not supported`)
+}