testing/checkers: make LogMatches inexact

testing/checkers: make LogMatches inexact

In the rare case that we want to check log messages,
we want to check a particular message has been logged,
not check the entirety of the log. This allows for 
changes (unrelated additions/removals) in logging
without affecting tests.

https://code.launchpad.net/~axwalk/juju-core/logmatches-inexact/+merge/191092

(do not edit description out of merge proposal)

[axw, 2013-10-15 02:49:08]

Please take a look.

[rog, 2013-10-15 08:54:03]

Great stuff - you beat me to it!

LGTM modulo the below suggestions and remarks.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go
File testing/checkers/log.go (right):

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode47
testing/checkers/log.go:47: expected[i].Message = s
I think I'd like to see Level set explicitly to
UNSPECIFIED here, so that I don't need to
check to see if it's zero when checking
the logic below.

Perhaps:
expected[i] = SimpleMessage{
    Message: s,
    Level: loggo.UNSPECIFIED,
}

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode56
testing/checkers/log.go:56: if expected[0].Level != loggo.UNSPECIFIED {
expect := expected[0]
if expect.Level != loggo.UNSPECIFIED && msg.Level != expect.Level {
    continue
}

might possibly read slightly better?
and then you can use expect rather than expected[0]
in a few places below too.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode61
testing/checkers/log.go:61: re := regexp.MustCompile(expected[0].Message)
I'm not sure we really want to panic if someone gets a pattern
wrong in the expected log messages.

How about:

ok, err := regexp.MatchString(expected[0].Message, msg.Message)
if err != nil {
    return false, "bad message regexp %q: %v", expected[0].Message, err)
}
if !ok {
    continue
}

?

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode80
testing/checkers/log.go:80: // expected messages; the specified messages will be
matched to the left-most
I think you can lose the part after the semicolon - the semantics are the same
whether it's leftmost or rightmost match or anything in between, aren't they?

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log_test.go
File testing/checkers/log_test.go (right):

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log_test.go#n...
testing/checkers/log_test.go:65: c.Check(log, gc.Not(jc.LogMatches),
[]string{"67890", ".*"})
I'd like to see at least one test that tests log level matching in an inexact
context.

Also, perhaps a test that checks that {".*", "foo bar"} doesn't match.

[axw, 2013-10-16 02:09:55]

Please take a look.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go
File testing/checkers/log.go (right):

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode47
testing/checkers/log.go:47: expected[i].Message = s
On 2013/10/15 08:54:04, rog wrote:
> I think I'd like to see Level set explicitly to
> UNSPECIFIED here, so that I don't need to
> check to see if it's zero when checking
> the logic below.
> 
> Perhaps:
> expected[i] = SimpleMessage{
>     Message: s,
>     Level: loggo.UNSPECIFIED,
> }

Done.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode56
testing/checkers/log.go:56: if expected[0].Level != loggo.UNSPECIFIED {
On 2013/10/15 08:54:04, rog wrote:
> expect := expected[0]
> if expect.Level != loggo.UNSPECIFIED && msg.Level != expect.Level {
>     continue
> }
> 
> might possibly read slightly better?
> and then you can use expect rather than expected[0]
> in a few places below too.

Done.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode61
testing/checkers/log.go:61: re := regexp.MustCompile(expected[0].Message)
On 2013/10/15 08:54:04, rog wrote:
> I'm not sure we really want to panic if someone gets a pattern
> wrong in the expected log messages.
> 
> How about:
> 
> ok, err := regexp.MatchString(expected[0].Message, msg.Message)
> if err != nil {
>     return false, "bad message regexp %q: %v", expected[0].Message, err)
> }
> if !ok {
>     continue
> }
> 
> ?

Done.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log.go#newcode80
testing/checkers/log.go:80: // expected messages; the specified messages will be
matched to the left-most
On 2013/10/15 08:54:04, rog wrote:
> I think you can lose the part after the semicolon - the semantics are the same
> whether it's leftmost or rightmost match or anything in between, aren't they?

Indeed, I didn't think that through properly. Removed, thanks.

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log_test.go
File testing/checkers/log_test.go (right):

https://codereview.appspot.com/14430064/diff/1/testing/checkers/log_test.go#n...
testing/checkers/log_test.go:65: c.Check(log, gc.Not(jc.LogMatches),
[]string{"67890", ".*"})
On 2013/10/15 08:54:04, rog wrote:
> I'd like to see at least one test that tests log level matching in an inexact
> context.
> 
> Also, perhaps a test that checks that {".*", "foo bar"} doesn't match.

Done.