Issue 6348053: state/ZooKeeper test consistency
Descriptionstate/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.
https://code.launchpad.net/~fwereade/juju-core/vast-zookeeper-tests-cleanup/+merge/112721
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix state and zookeeper tests #Patch Set 3 : state/ZooKeeper test consistency #Patch Set 4 : state/ZooKeeper test consistency #
Total comments: 10
Patch Set 5 : state/ZooKeeper test consistency #
Total comments: 14
Patch Set 6 : state/ZooKeeper test consistency #
MessagesTotal messages: 7
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
