Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(625)

Issue 7308080: state: remove AddUnitSubordinateTo

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by fwereade
Modified:
11 years, 1 month ago
Reviewers:
mp+147550, dimitern, jameinel
Visibility:
Public.

Description

state: remove AddUnitSubordinateTo This involved entirely replacing the machine units watcher tests. They're noticeably simplified; I don't think that I have reduced coverage, because all the important considerations are still handled; I've just dropped all the redundant coverage of multiple changes for each operation. If I've missed something, please let me know. https://code.launchpad.net/~fwereade/juju-core/kill-aust/+merge/147550 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : state: remove AddUnitSubordinateTo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+246 lines, -735 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M state/export_test.go View 2 chunks +0 lines, -48 lines 0 comments Download
M state/machine_test.go View 2 chunks +234 lines, -668 lines 0 comments Download
M state/relationunit.go View 1 chunk +1 line, -1 line 0 comments Download
M state/service.go View 3 chunks +9 lines, -18 lines 0 comments Download

Messages

Total messages: 4
fwereade
Please take a look.
11 years, 1 month ago (2013-02-10 10:19:13 UTC) #1
dimitern
LGTM, Nice! I have a couple of trivial suggesstions. https://codereview.appspot.com/7308080/diff/1/state/machine_test.go File state/machine_test.go (right): https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode473 state/machine_test.go:473: ...
11 years, 1 month ago (2013-02-10 10:25:10 UTC) #2
jameinel
LGTM https://codereview.appspot.com/7308080/diff/1/state/machine_test.go File state/machine_test.go (right): https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode286 state/machine_test.go:286: rel, err := s.State.AddRelation(eps...) I'm probably mixing my ...
11 years, 1 month ago (2013-02-11 10:10:12 UTC) #3
fwereade
11 years, 1 month ago (2013-02-11 11:22:22 UTC) #4
*** Submitted:

state: remove AddUnitSubordinateTo

This involved entirely replacing the machine units watcher tests. They're
noticeably simplified; I don't think that I have reduced coverage, because
all the important considerations are still handled; I've just dropped all
the redundant coverage of multiple changes for each operation. If I've
missed something, please let me know.

R=dimitern, jameinel
CC=
https://codereview.appspot.com/7308080

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

https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode286
state/machine_test.go:286: rel, err := s.State.AddRelation(eps...)
On 2013/02/11 10:10:12, jameinel wrote:
> I'm probably mixing my patches up a bit, but isn't this where you wanted to
have
> an AddRelation that would do the endpoint inferring? (It can certainly happen
in
> another patch)

That's the plan, but it's something I intend to do in my Copious Free Time --
there are more serious races to worry about before I work on that.

https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode473
state/machine_test.go:473: assertClosed := func() {
On 2013/02/10 10:25:10, dimitern wrote:
> Maybe move this at the top with the other asserts?

Longer-term, I'm hoping to pull all these
asertChanged/assertNoChange/assertClosed functions out into global scope; I
thought this wasn't the CL for that change, though.

Note that it's not actually reused, and probably shouldn't be a function in this
local context; the intent is to make it easier to extract when I'm ready, and
for now to keep its definition local to its use.

https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode502
state/machine_test.go:502: assertChange("mysql/1")
On 2013/02/11 10:10:12, jameinel wrote:
> This could be out of scope for this proposal, but it sure would be nice if all
> these asserts could be decoupled into separate test cases, so the state of one
> assertion is not tied to the previous one. (Otherwise you make a change, and
> suddenly the assert at the end fails because of a different precondition.)

Understood; it's on my mind. I'm just not sure that making our tests saner is
our top priority at the moment, and this is very specifically targeted at
removing a single infelicity. (separate test cases imply pulling out the
asserts; pulling out the asserts implies a much wider change (or just one more
inconsistency with no guarantees that it'll get fixed any time soon)).

https://codereview.appspot.com/7308080/diff/1/state/machine_test.go#newcode589
state/machine_test.go:589: assertClosed := func() {
On 2013/02/10 10:25:10, dimitern wrote:
> Ditto

See above.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b