Merge lp://qastaging/~rogpeppe/gocheck/missing-method-panic into lp://qastaging/gocheck

Reviewers: mp+121193_code.launchpad.net,

Message:
Please take a look.

Description:
gocheck: diagnose missing methods

Because of the way that gocheck works, there's no
diagnostic if methods are accidentally omitted through
a type-related issue. This proposal changes gocheck
so that such methods are treated as if they had panicked.
It diagnosed three such examples in the current juju trunk.

Two examples that are now diagnosed:

1) Methods which are missing because they're defined
as pointer methods but the value passed to Suite is
a value type.

 type mySuite struct{}
 func (s *mySuite) Test1(c *C) {}
 var _ = Suite(mySuite{})

This gives the message:
----------------------------------------------------------------------
PANIC: foo_test.go:9: mySuite.Test1

... Panic: pointer method mySuite.Test1 defined on value type

2) Fixture methods which are missing because
two fixture suites have been embedded.

 type helperSuite1 struct{}
 func (helperSuite1) SetUpTest(c *C) {}

 type helperSuite2 struct{}
 func (helperSuite2) SetUpTest(c *C) {}

 type mySuite struct {
  helperSuite1
  helperSuite2
 }
 func (s mySuite) Test1(c *C) {}

 var _ = Suite(mySuite{})

This gives the message:

----------------------------------------------------------------------
PANIC: foo_test.go:9: helperSuite1.SetUpTest

... Panic: clashing definitions of mySuite.SetUpTest:
mySuite.helperSuite1.SetUpTest, mySuite.helperSuite2.SetUpTest

----------------------------------------------------------------------
PANIC: foo_test.go:18: mySuite.Test1

... Panic: Fixture has panicked (see related PANIC)

The testing for this feature is not yet complete, but I don't
want to spend any more time on it if it's not considered a
good way forward.

The change does also involve one change to the current
semantics - if a method has been defined with an
incorrect type signature, it will be treated as if it
panics in the same way as above, as it seems consistent
with the above behaviour (and it simplifies the code).
It would be easy to reinstate the old behaviour if desired
(the relevant tests are skipped pending a decision).

https://code.launchpad.net/~rogpeppe/gocheck/missing-method-panic/+merge/121193

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M fixture_test.go
   M gocheck.go
   M gocheck_test.go
   M run.go
   M run_test.go

I appreciate the goal and the outcome, but I'm surprised with the size
of the change. Right now we pick methods up with about 20 lines of code.
This is adding hundreds of lines of logic just to *validate* that the
method is good? Why is it that hard?

https://codereview.appspot.com/6485060/

It is hard because the reflect package, not surprisingly, doesn't make it
easy to find methods which have been hidden. If you can think of a more
straightforward way, that would be great.
On Aug 24, 2012 7:30 PM, <email address hidden> wrote:

> I appreciate the goal and the outcome, but I'm surprised with the size
> of the change. Right now we pick methods up with about 20 lines of code.
> This is adding hundreds of lines of logic just to *validate* that the
> method is good? Why is it that hard?
>
> https://codereview.appspot.**com/6485060/<https://codereview.appspot.com/6485060/>
>

The position is still the same as the last comment: this is a nice feature, but a heavy implementation. Hopefully we'll figure a way to do that with a cost that is compatible with the benefit in the future.

The position is still the same as the last comment: this is a nice
feature, but a heavy implementation. Hopefully we'll figure a way to do
that with a cost that is compatible with the benefit in the future.

https://codereview.appspot.com/6485060/