state: testable transactions

state: testable transactions

Includes sample tests; actually testing everything properly will be a much
bigger task...

https://code.launchpad.net/~fwereade/juju-core/state-testable-transactions/+merge/162369

(do not edit description out of merge proposal)

[fwereade, 2013-05-03 14:57:58]

Please take a look.

[dimitern, 2013-05-03 15:06:29]

Excellent, LGTM. This really changes how state tests can be written!

https://codereview.appspot.com/9175043/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/9175043/diff/1/state/machine_test.go#newcode82
state/machine_test.go:82: func (s *MachineSuite) TestDestroyCancel(c *C) {
very good!

https://codereview.appspot.com/9175043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9175043/diff/1/state/state.go#newcode154
state/state.go:154: panic("concurrent use of transaction hooks")
this is to safeguard against txns running in goroutines, right?

[jameinel, 2013-05-07 16:10:01]

I really like that this makes it possible to inject stuff into the system, so
that you can play around with ensuring transaction safety.

I'm concerned a little bit about the magic/side effects of it.

Specifically:

Using a channel to buffer the transactions to run, popping off the slice, and
then popping it on and off again seems a bit strange.

Especially given that every run has to pop nil onto the channel, and then pop it
off again. Which means that your actual test suite assertion "HasLen(<-, 0)"
could be accidentally passed. If you get to the beginning of runTxn but you
don't get a chance to pop the remainder of the slice back on.

Also, the specific syntax of "runStart, runEnd, runStart, runEnd" is a little
confusing (Is this an even or odd parameter next?)
And the fact that you don't name the transaction being run. (so if you add a
transaction as part of an operation, the hooks will be run at the wrong time,
without any indication of what is going wrong).

This certainly doesn't have to be a blocker, but do you think it would be
clearer to name the transactions, and then use a syntax to make it clear what
transaction you are trying to trap and what the setup and teardown hooks are?

Something like:

type TxnHook struct {
 Name string
 Setup func()
 TearDown func()
}

func SetTxnHooks(..., TxnHook...) {
}

func TestFoo(...) {
  defer SetTxnHooks(stuff, 
    TxnHook{Name: "InsertRecord", Setup: func() {}})()
}

func TestRepeats(...) {
  txnHook := TxnHook{Name: "UpdateRecord", Setup: assign, TearDown: Unassign}
  defer SetTxnHooks(stuff, txnHook, txnHook, txnHook)()
}


I'm not 100% sure this is better, but it might help with the:
1) What transaction is this test trying to trap
2) Am I passing a setup or a teardown hook
3) Are there any hooks remaining that haven't been run?

What do you think? I'd probably rather see this land than not, but I do find it
a bit hard to follow what is getting setup, and what hook ends up getting run
when.

[thumper, 2013-05-09 16:13:58]

https://codereview.appspot.com/9175043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9175043/diff/1/state/state.go#newcode144
state/state.go:144: func (st *State) runTxn(ops []txn.Op) error {
I have to say that this function name has one of my pet hates.  Any reason why
this can't be runTransaction?  Can we start using full words?

[fwereade, 2013-05-15 09:17:39]

On 2013/05/07 16:10:01, jameinel wrote:
> I really like that this makes it possible to inject stuff into the system, so
> that you can play around with ensuring transaction safety.
> 
> I'm concerned a little bit about the magic/side effects of it.
> 
> Specifically:
> 
> Using a channel to buffer the transactions to run, popping off the slice, and
> then popping it on and off again seems a bit strange.

It's a useful idiom for a self-mutexed variable. It's intended to be clean and
cheap in real usage (N goroutines using the same state object, all just
pop/pushing nil) but useful in testing (more than one goroutine in play renders
this technique worthless anyway). Suggestions for improvements here gratefully
accepted, of course.

> Especially given that every run has to pop nil onto the channel, and then pop
it
> off again. Which means that your actual test suite assertion "HasLen(<-, 0)"
> could be accidentally passed. If you get to the beginning of runTxn but you
> don't get a chance to pop the remainder of the slice back on.

I *think* this will only happen when you're running tests and code on different
goroutines, so I'm not really bothered here -- it's not sane to do that in the
first place.

> Also, the specific syntax of "runStart, runEnd, runStart, runEnd" is a little
> confusing (Is this an even or odd parameter next?)

Yeah, I was on the fence here. Justified below? Maybe not.

> And the fact that you don't name the transaction being run. (so if you add a
> transaction as part of an operation, the hooks will be run at the wrong time,
> without any indication of what is going wrong).

It went wrong when someone wrote a state method that used more than one
transaction in the first place, I think, so I'm not too concerned here.

> This certainly doesn't have to be a blocker, but do you think it would be
> clearer to name the transactions, and then use a syntax to make it clear what
> transaction you are trying to trap and what the setup and teardown hooks are?
> 
> ...
> 
> I'm not 100% sure this is better, but it might help with the:
> 1) What transaction is this test trying to trap

The one you're testing... it should always be the "same" transaction, even if
its form differs according to changing conditions. I'd consider it abuse to set
up txn hooks for a test that used multiple transactions -- you *could* do it,
but it implies that you're working at the wrong level. I think.

> 2) Am I passing a setup or a teardown hook

FWIW, I'd call them "before"/"after" -- I don't think setup/teardown quite
capture intent. And 99% of the time, I think, you're just passing a single
"before" hook. The only real use case for multiples is to
break/unbreak/break/unbreak/break/unbreak until ErrExcessiveContention, and I
think the existing syntax is adequate there.

But I'll give it a try with a {Before, After} struct and see how I go.

> 3) Are there any hooks remaining that haven't been run?

I think this is moot, but I may not be following you properly. Expand please?

> What do you think? I'd probably rather see this land than not, but I do find
it
> a bit hard to follow what is getting setup, and what hook ends up getting run
> when.

I'm pretty sure that state methods should in general run zero or one
transactions -- any other number allows for half-completed operations, and I'd
rather demote any such to another package in the service of consistency and
clarity in state. With this assumption made explicit, does my aproach make a bit
more sense?

[fwereade, 2013-05-15 09:29:22]

https://codereview.appspot.com/9175043/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/9175043/diff/1/state/machine_test.go#newcode82
state/machine_test.go:82: func (s *MachineSuite) TestDestroyCancel(c *C) {
On 2013/05/03 15:06:29, dimitern wrote:
> very good!
:D

https://codereview.appspot.com/9175043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/9175043/diff/1/state/state.go#newcode144
state/state.go:144: func (st *State) runTxn(ops []txn.Op) error {
On 2013/05/09 16:13:58, thumper wrote:
> I have to say that this function name has one of my pet hates.  Any reason why
> this can't be runTransaction?  Can we start using full words?

I'm fine with full names in general. Keep hassling me when I forget, it'll
become second nature again one day ;).

https://codereview.appspot.com/9175043/diff/1/state/state.go#newcode154
state/state.go:154: panic("concurrent use of transaction hooks")
On 2013/05/03 15:06:29, dimitern wrote:
> this is to safeguard against txns running in goroutines, right?

Yeah -- concurrent use of this mechanism is not sane, and I'm fine with
panicking the odd insane test. Should be perfectly safe otherwise. (I don't
think it'd be sane to implement this without the buffered txnHooks channel,
though -- it has to be sane in concurrent usage when there aren't any hooks in
play.)

[fwereade, 2013-06-13 14:21:21]

Please take a look.

[fwereade, 2013-06-13 14:26:03]

On 2013/06/13 14:21:21, fwereade wrote:
> Please take a look.

This roughly incorporates some of jam's thoughts without going all the way.

[jameinel, 2013-06-14 07:01:58]

I like the more explicit Before and After stuff. I think most of this is still
just mechanical to switch to runTransaction.
LGTM