Repository: cassandra Updated Branches: refs/heads/trunk b87f79798 -> 7583c9b96
Add in-tree testing guidelines Patch by Blake Eggleston; Reviewed by Jason Brown for CASSANDRA-13497 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/7583c9b9 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/7583c9b9 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/7583c9b9 Branch: refs/heads/trunk Commit: 7583c9b96af53e9b004ede40123da187d74440f3 Parents: b87f797 Author: Blake Eggleston <[email protected]> Authored: Sun Apr 23 19:25:32 2017 -0700 Committer: Blake Eggleston <[email protected]> Committed: Mon May 15 13:57:56 2017 -0700 ---------------------------------------------------------------------- CHANGES.txt | 1 + CONTRIBUTING.md | 1 + TESTING.md | 448 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 450 insertions(+) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/7583c9b9/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 1191086..03870dd 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0 + * Add testing guidelines (CASSANDRA-13497) * Add more repair metrics (CASSANDRA-13531) * RangeStreamer should be smarter when picking endpoints for streaming (CASSANDRA-4650) * Avoid rewrapping an exception thrown for cache load functions (CASSANDRA-13367) http://git-wip-us.apache.org/repos/asf/cassandra/blob/7583c9b9/CONTRIBUTING.md ---------------------------------------------------------------------- diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8366579..25e15ee 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,3 +15,4 @@ Use [Cassandra JIRA](https://issues.apache.org/jira/browse/CASSANDRA/) to create - Running Cassandra in Eclipse [guide](https://wiki.apache.org/cassandra/RunningCassandraInEclipse) - Cassandra Cluster Manager - [CCM](https://github.com/pcmanus/ccm) and a guide [blog post](http://www.datastax.com/dev/blog/ccm-a-development-tool-for-creating-local-cassandra-clusters) - Cassandra Distributed Tests aka [dtests](https://github.com/riptano/cassandra-dtest) +- Cassandra Testing Guidelines - see TESTING.md http://git-wip-us.apache.org/repos/asf/cassandra/blob/7583c9b9/TESTING.md ---------------------------------------------------------------------- diff --git a/TESTING.md b/TESTING.md new file mode 100644 index 0000000..de0c34a --- /dev/null +++ b/TESTING.md @@ -0,0 +1,448 @@ +The goal of this document is to establish guidelines on writing tests that drive incremental improvement of the test coverage and testability of +Cassandra, without overly burdening day to day work. While not every point here will be immediately practical to implement or relevant for every +contribution, it errs on the side of not making rules out of potential exceptions. It provides guidelines on test scope, style, and goals, as +weel as guidelines for dealing with global state and refactoring untested code. + +## What to Test + +There are 3 main types of tests in Cassandra, unit tests, integration tests, and dtests. Below, each type of test is described, and a high level description of +what should and shouldn't be tested in each is given. + +### Unit Tests +JUnit tests of smaller components that are fairly narrow in scope (ie: data structures, verb handlers, helper classes) + +#### What should be tested + * All state transitions should be tested + * Illegal state transitions should be tested (that they throw exceptions) + * all conditional branches should be tested. + * Code that deals with ranges of values should have tests around expected ranges, unexpected ranges, different functional ranges and their boundaries. + * Exception handling should be tested. + +#### What shouldn't be tested +* implementation details (test that the system under test works a certain way, not that it's implemented a certain way) + +### Integration Tests +JUnit tests of larger components with a lot of moving parts, usually involved in some internode communication (ie: Gossip, MessagingService). +The smaller components that make up the system under test in an integration test should be individually unit tested. + +#### What should be tested +* messages are sent when expected +* received messages have the intended side effects +* internal interfaces work as expected +* external interfaces are interacted with as expected +* multiple instances of components interact as expected (with a mocked messaging service, and other dependencies, where appropriate) +* dry start - test that a system starts up properly the first time a node start +* restart - test that a system starts up properly on node restart (from both clean and unclean shutdown) +* shutdown - test that a system can shutdown properly +* upgrade - test that a system is able to restart with data from a previous version + +#### What shouldn't be tested +* The rest of the application. It should be possible to test large systems without starting the entire database through the use of mocks. + +**Note:** it's generally not a good idea to mock out the storage layer if the component under test needs to interact with it. If you're testing +how multiple instances interact with each other, AND they need to use the storage layer, parameterizing the keyspace/table they store data is +the way to do it. + +### dtests +python/ccm tests that start local clusters and interact with them via the python client. + +dtests are effectively black box tests, and should be used for checking that clusters and client side interfaces work as expected. They are not +a replacement for proper functional java tests. They take much longer to run, and are much less flexible in what they can test. + +Systems under test in dtest should have more granular integration tests as well. + +#### What should be tested +* end to end cluster functionality +* client contracts +* trivial (to create) failure cases + +#### What shouldn't be tested +* internal implementation details + +## Expanded Guidelines + +This section has more in depth descriptions and reasoning about some of the points raised in the previous section. + +### Test structure + +Tests cases should have a clear progression of setup, precondition check, performing some action under test, then a postcondition check. + +**Example** + +```java +@Test +public void increment() throws Exception +{ + // setup code + int x = 1; + + // check preconditions + assertEquals(1, x); + + // perform the state changing action under test + x++; + + // check post conditions + assertEquals(2, x); +} +``` + +#### Reason + +Test cases should be optimized for readability + +#### Exceptions + +Cases where simple cases can be tested in one line. Such as validation logic checks: +property-based state testing (ie: ScalaCheck/QuickCheck) + +*Example:* +```java +@Test +public void validation() +{ + assertValidationFailure(b -> b.withState(null)); + assertValidationFailure(b -> b.withSessionID(null)); + assertValidationFailure(b -> b.withCoordinator(null)); + assertValidationFailure(b -> b.withTableIds(null)); + assertValidationFailure(b -> b.withTableIds(new HashSet<>())); + assertValidationFailure(b -> b.withRepairedAt(0)); + assertValidationFailure(b -> b.withRepairedAt(-1)); + assertValidationFailure(b -> b.withRanges(null)); + assertValidationFailure(b -> b.withRanges(new HashSet<>())); + assertValidationFailure(b -> b.withParticipants(null)); + assertValidationFailure(b -> b.withParticipants(new HashSet<>())); + assertValidationFailure(b -> b.withStartedAt(0)); + assertValidationFailure(b -> b.withLastUpdate(0)); +} +``` + +### Test distributed components in junit + +Components that rely on nodes communicating with each other should be testable in java. + +#### Reason + +One of the more difficult aspects of distributed systems is ensuring that they continue to behave correctly during various failure modes. This includes weird +edge cases involving specific ordering of events between nodes that rarely occur in the wild. Testing these sorts of scenarios is much easier in junit because +mock 'clusters' can be placed in specific states, and deterministically stepped through a sequence of events, ensuring that they behave as expected, and are in +the expected state after each step. + +#### Exceptions + +This rule mainly applies to new or significantly overhauled systems. Older systems *should* be refactored to be thoroughly tested, but it's not necessarily a +prerequisite for working on them. + +### Test all branches and inputs. + +All branches and inputs of a method should be exercised. For branches that require multiple criteria to be met, (ie `x > 10 && y < 100`), there +should be tests demonstrating that the branch is taken when all critera are met (ie `x=11,y=99`), and that it is not taken when only one is met +(ie: `x=11, y=200 or x=5,y=99`). If a method deals with ranges of values, (ie `x >= 10`), the boundaries of the ranges should be tested (ie: `x=9, x=10`) + +In the following example + +**Example** +```java +class SomeClass +{ + public static int someFunction(bool aFlag, int aValue) + { + if (aFlag && aValue > 10) + { + return 20; + } + else if (aValue > 5) + { + return 10; + else + { + return 0; + } + } +} + +class SomeTest +{ + public void someFunction() throws Exception + { + assertEquals(10, somefunction(true, 11)); + assertEquals(5, somefunction(false, 11)); + assertEquals(5, somefunction(true, 8)); + assertEquals(5, somefunction(false, 8)); + assertEquals(0, somefunction(false, 4)); + } +} +``` + +### Test any state transitions + +As an extension of testing all branches and inputs. For stateful systems, there should be tests demonstrating that states change under the intended +circumstances, and that state changes have the intended side effects. + +### Test unsupported arguments and states throw exceptions + +If a system is not intended to perform an action in a given state (ie: a node performing reads during bootstrap), or a method is not intended +to encounter some type of argument (ie: a method that is only designed to work with numeric values > 0), then there should be tests demonstrating +that an appropriate exception is raised (IllegalStateException or IllegalArgumentException, respectively) in that case. + +The guava preconditions module makes this straightforward. + +#### Reason + +Inadvertent misuse of methods and systems cause bugs that are often silent and subtle. Raising exceptions on unintended usage helps +protect against future bugs and reduces developer surprise. + +## Dealing with global state + +Unfortunately, the project has extensive amounts of global state which makes actually writing robust tests difficult, but not impossible. + +Having dependencies on global state is not an excuse to not test something, or to throw a dtest or assertion at it and call it a day. + +Structuring code in a way that interacts with global state that can still be deterministically tested just takes a few tweaks + +**Example, bad** +```java +class SomeVerbHandler implements IVerbHandler<SomeMessage> +{ + public void doVerb(MessageIn<SomeMessage> msg) + { + if (FailureDetector.instance.isAlive(msg.payload.otherNode)) + { + new StreamPlan(msg.payload.otherNode).requestRanges(someRanges).execute(); + } + else + { + CompactionManager.instance.submitBackground(msg.payload.cfs); + } + } +} +``` + +In this made up example, we're checking global state, and then taking some action against other global state. None of the global state +we're working with is easy to manipulate for tests, so comprehensive tests for this aren't very likely to be written. Even worse, whether +the FailureDetector, streaming, or compaction work properly are out of scope for our purposes. We're concerned with whether SomeVerbHandler +works as expected. + +Ideally though, classes won't have dependencies on global state at all, and have everything they need to work passed in as constructor arguments. +This also enables comprehensive testing, and stops the spread of global state. + +This prevents the spread of global state, and also begins to identify and define the internal interfaces that will replace global state. + +**Example, better** +```java +class SomeVerbHandler implements IVerbHandler<SomeMessage> +{ + private final IFailureDetector failureDetector; + private final ICompactionManager compactionManager; + private final IStreamManager streamManager; + + public SomeVerbHandler(IFailureDetector failureDetector, ICompactionManager compactionManager, IStreamManager streamManager) + { + this.failureDetector = failureDetector; + this.compactionManager = compactionManager; + this.streamManager = streamManager; + } + + public void doVerb(MessageIn<SomeMessage> msg) + { + if (failureDetector.isAlive(msg.payload.otherNode)) + { + streamExecutor.submitPlan(new StreamPlan(msg.payload.otherNode).requestRanges(someRanges)); + } + else + { + compactionManager.submitBackground(msg.payload.cfs); + } + } +} +``` + +**Example test** +```java +class SomeVerbTest +{ + class InstrumentedFailureDetector implements IFailureDetector + { + boolean alive = false; + @Override + public boolean isAlive(InetAddress address) + { + return alive; + } + } + + class InstrumentedCompactionManager implements ICompactionManager + { + boolean submitted = false; + @Override + public void submitBackground(ColumnFamilyStore cfs) + { + submitted = true; + } + } + + class InstrumentedStreamManager implements IStreamManager + { + boolean submitted = false; + @Override + public void submitPlan(StreamPlan plan) + { + submitted = true; + } + } + + @Test + public void liveNode() throws Exception + { + InstrumentedFailureDetector failureDetector = new InstrumentedFailureDetector(); + failureDetector.alive = true; + InstrumentedCompactionManager compactionManager = new InstrumentedCompactionManager(); + InstrumentedStreamManager streamManager = new InstrumentedStreamManager(); + SomeVerbHandler handler = new SomeVerbHandler(failureDetector, compactionManager, streamManager); + + MessageIn<SomeMessage> msg = new MessageIn<>(...); + + assertFalse(streamManager.submitted); + assertFalse(compactionManager.submitted); + + handler.doVerb(msg); + + assertTrue(streamManager.submitted); + assertFalse(compactionManager.submitted); + } + + @Test + public void deadNode() throws Exception + { + InstrumentedFailureDetector failureDetector = new InstrumentedFailureDetector(); + failureDetector.alive = false; + InstrumentedCompactionManager compactionManager = new InstrumentedCompactionManager(); + InstrumentedStreamManager streamManager = new InstrumentedStreamManager(); + SomeVerbHandler handler = new SomeVerbHandler(failureDetector, compactionManager, streamManager); + + MessageIn<SomeMessage> msg = new MessageIn<>(...); + + assertFalse(streamManager.submitted); + assertFalse(compactionManager.submitted); + + handler.doVerb(msg); + + assertFalse(streamManager.submitted); + assertTrue(compactionManager.submitted); + } +} +``` + +By abstracting away accesses to global state we can exhaustively test the paths this verb handler can take, and directly confirm that it's taking the correct +actions. Obviously, this is a simple example, but for classes or functions with more complex logic, this makes comprehensive testing much easier. + +Note that the interfaces used here probably shouldn't be the same ones we use for MBeans. + +However, in some cases, passing interfaces into the constructor may not be practical. Classes that are instantiated on startup need to be handled with care, since accessing +a singleton may change the initialization order of the database. It may also be a larger change than is warranted for something like a bug fix. In any case, if passing +dependencies into the constructor is not practical, wrapping accesses to global state in protected methods that are overridden for tests will achieve the same thing. + + +**Example, alternative** +```javayy +class SomeVerbHandler implements IVerbHandler<SomeMessage> +{ + @VisibleForTesting + protected boolean isAlive(InetAddress addr) { return FailureDetector.instance.isAlive(msg.payload.otherNode); } + + @VisibleForTesting + protected void streamSomethind(InetAddress to) { new StreamPlan(to).requestRanges(someRanges).execute(); } + + @VisibleForTesting + protected void compactSomething(ColumnFamilyStore cfs ) { CompactionManager.instance.submitBackground(); } + + public void doVerb(MessageIn<SomeMessage> msg) + { + if (isAlive(msg.payload.otherNode)) + { + streamSomething(msg.payload.otherNode); + } + else + { + compactSomething(); + } + } +} +``` + +**Example test** +```java +class SomeVerbTest +{ + static class InstrumentedSomeVerbHandler extends SomeVerbHandler + { + public boolean alive = false; + public boolean streamCalled = false; + public boolean compactCalled = false; + + @Override + protected boolean isAlive(InetAddress addr) { return alive; } + + @Override + protected void streamSomethind(InetAddress to) { streamCalled = true; } + + @Override + protected void compactSomething(ColumnFamilyStore cfs ) { compactCalled = true; } + } + + @Test + public void liveNode() throws Exception + { + InstrumentedSomeVerbHandler handler = new InstrumentedSomeVerbHandler(); + handler.alive = true; + MessageIn<SomeMessage> msg = new MessageIn<>(...); + + assertFalse(handler.streamCalled); + assertFalse(handler.compactCalled); + + handler.doVerb(msg); + + assertTrue(handler.streamCalled); + assertFalse(handler.compactCalled); + } + + @Test + public void deadNode() throws Exception + { + InstrumentedSomeVerbHandler handler = new InstrumentedSomeVerbHandler(); + handler.alive = false; + MessageIn<SomeMessage> msg = new MessageIn<>(...); + + assertFalse(handler.streamCalled); + assertFalse(handler.compactCalled); + + handler.doVerb(msg); + + assertFalse(handler.streamCalled); + assertTrue(handler.compactCalled); + } +} +``` + +## Refactoring Existing Code + +If you're working on a section of the project that historically hasn't been well tested, it will likely be more difficult for you to write tests around +whatever you're doing, since the code may not have been written with testing in mind. You do need to add tests around the subject of you're +jira, and this will probably involve some refactoring, but you're also not expected to completely refactor a huge class to submit a bugfix. + +Basically, you need to be able to verify the behavior you intend to modify before and after your patch. The amount of testing debt you pay back should be +roughly proportional to the scope of your change. If you're doing a small bugfix, you can get away with refactoring just enough to make the subject of your +fix testable, even if you start to get into 'testing implementation details' territory'. The goal is incremental improvement, not making things perfect on +the first iteration. If you're doing something more ambitious though, you may have to do some extra work to sufficiently test your changes. + +## Refactoring Untested Code + +There are several components that have very little, if any, direct test coverage. We really should try to improve the test coverage of these components. +For people interested in getting involved with the project, adding tests for these is a great way to get familiar with the codebase. + +First, get feedback on the subject and scope of your proposed refactor, especially larger ones. The smaller and more narrowly focused your proposed +refactor is, the easier it will be for you to get it reviewed and committed. + +Start with smaller pieces, refactor and test them, and work outwards, iteratively. Preferably in several jiras. Ideally, each patch should add some value +to the project on it's own in terms of test coverage. Patches that are heavy on refactoring, and light on tests are not likely to get committed. People come and go +from projects, and having a many small improvements is better for the project than several unfinished or ongoing refactors that don't add much test coverage. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
