295.1.1
|
|
|
Dave Cheney |
11 years ago
|
|
|
295
|
|
|
Aram Hăvărneanu |
11 years ago
|
|
|
294
|
|
|
Aram Hăvărneanu |
11 years ago
|
|
|
293
|
|
|
Aram Hăvărneanu |
11 years ago
|
|
|
292
|
|
|
Aram Hăvărneanu |
11 years ago
|
|
|
291
|
|
|
Aram Hăvărneanu |
11 years ago
|
|
|
290
|
|
|
Dave Cheney |
11 years ago
|
|
|
289
|
|
|
Frank Mueller |
11 years ago
|
|
|
288
|
|
|
Dave Cheney |
11 years ago
|
|
|
287
|
|
|
William Reade |
11 years ago
|
|
|
286
|
|
|
Dave Cheney |
11 years ago
|
|
|
285
|
|
|
William Reade |
11 years ago
|
|
|
284
|
|
|
Dave Cheney |
11 years ago
|
|
|
283
|
|
|
William Reade |
11 years ago
|
|
|
282
|
|
|
Dave Cheney |
11 years ago
|
|
|
281
|
|
state/ZooKeeper test consistency
OK, this is a *monster* of a change, and I'm sorry for that; but the niggling frustrations of duplicated and subtly inconsistent test code across packages, and of the poor organisation of the state tests, have become a serious mental drain. All the following changes are interrelated to a greater or lesser extent, and my judgment is that while I could express this change as a long pipeline it would in fact be counterproductive to do so; a big bang merge is actually going to save us all time.
Yes, even you, dear reviewer. This will be explained shortly; in the meantime, here's a sketch of the changes in this CL:
1) ZooKeeper setup and maintenance has been consolidated
Every test package that wants to use a ZooKeeper server should register its tests by calling testing.ZkTestPackage; every such test suite should embed and use testing.ZkSuite, which will clear out the server at the end of each test. See testing/zk.go.
Note that this simplifies the use of the dummy environ, which now expects client test packages to use ZkTestPackage, and determines the test server by inspecting the testing package directly; it also removes boilerplate from internal state tests, and from the tests for state's subpackages.
2) State test setup and maintenance has been consolidated
Every test that wishes to use a *state.State should embed and use state/testing.StateSuite, which will supply a freshly Initialized State to each test method.
This simplifies the testing for container, and for several cmd subpackages, not to mention for the state package itself.
3) StateSuite has been split up into broadly logical chunks
Essentially, StateSuite is for testing methods on State; it will occasionally touch instances of other types returned from State methods, but only in service of testing the State type. (oh, yeah, and testing the diff function, because I couldn't think of anywhere better to put the tests, which are certainly too small to deserve a file of their own).
Tests for other types are generally in their own files, with their own suites, and include the tests for the watchers returned by that type's methods; the commonality of implementation of watchers means it's sensible to keep the implementations in the same file, but there's little justification for smooshing all the tests together when each test is far more closely connected with the type that returns it than with the general concept of "watcher".
Exceptions to this rule are assign_test, which collects all the unit/machine assignment tests into one place; and relation_test, which should perhaps more properly be part of StateSuite according to the logic above, but which is in fact far more concerned with testing the details of the relations than with the fact that they happen to be created by State.
In general, the type-specific tests have had common setup code moved to SetUpTest and are otherwise unmodified; in a couple of cases, factoring the common behaviour into one place led to sufficiently weird/ugly looking tests that they have been modified slightly for readability.
A number of tests appears to be asserting more-or-less insane behaviour; these have been flagged with TODO BUG but I have no interest whatsoever in mixing functionality changes into this CL, on the basis that it would divert the team's precious time and creativity into groudbreaking methods of stabbing me over the intrnet.
4) state package internals have been rearranged slightly
This is mainly about the internal tests -- internal_test.go has been split into topology_test.go and confignode_test.go -- but I also took the opportunity to move ConfigNode, and supporting code, into a new confignode.go file, because it was taking up the vast majority of util.go, and the ickiness was again impossible to ignore.
5) cmd/jujuc/server.UnitFixture renamed to UnitSuite
For consistency's sake.
6) cmd/jujud/main_test moved into cmd/jujud
And is now actually run again.
7) state/presence test stability fixes
* hugely increasing pinger periods and timeouts to account for observed behaviour * adding diagnostic Debugfs that should be useful should we encounter further issues * rearranging Pinger.Stop and .Kill such that Kill can always be called safely * consequently adding defered Kills to all test Pingers, ensuring they don't live beyond their intended lifetime and cause cascading failures in other tests
~~~~~
Almost all the above changes are necesary for me to start working on state.Initialize, which is a prerequisite for correct handling of the default-series environment setting; as soon as I change its behaviour it will have far-reaching effects across the codebase, and the prospect of sorting those out without the above consolidations in place (and the prospect of the further subtle inconsistencies that would sneak in were I to attempt to do so) gives me the screaming heebie-jeebies.
The changes that are not strictly necessary have been done anyway, because it's incredibly hard to achieve consistency when aiming at a moving target. By imposing them all at the same time, we actually reach a consistent state and thereby have the best chance of maintaining it for a while, and saving ourselves the subtle but debilitating low-level headaches that come from an inadequately tended source tree. I hope.
~~~~~
There are a number of additional changes I have resisted the temptation to address; most importantly, the state_test.ConnSuite is problematic, because it is the single component that splurges zookeeper dependencies across all the state tests that depend on it. Our testing of underlying zookeeper state is extremely inconsistent, and isolating it felt too much like too much more hard work; but if we *can* isolate all the tests that inspect/modify zookeeper directly, then it should become possible to use the other, non-polluted, tests to verify both state and mstate; which would be *awesome*.
I also didn't hit mstate at all, because I suspect it's still in a somewhat churny situation; conversely, AFAIK, only myself and TheMue are currently touching state, and TheMue only with a single branch that's still WIP; my situation is more delicate, but I consider the slight extra hassle of resolving the conflicts to be a small price to pay for the benefits.
~~~~~
TLDR: we had to do this, and it's less complicated than it looks.
R=dfc, TheMue, niemeyer CC= https://codereview.appspot.com/6348053
|
William Reade |
11 years ago
|
|
|
280
|
|
|
Dave Cheney |
11 years ago
|
|
|
279
|
|
|
Dave Cheney |
11 years ago
|
|
|
278
|
|
|
Dave Cheney |
11 years ago
|
|
|
277
|
|
|
Dave Cheney |
11 years ago
|
|
|