Since we're here, do we have plan to integrate with a dependency injection framework like Dagger2? Otherwise it'll be difficult to write unit test cases.
On Thu, Mar 16, 2017 at 1:16 PM, Edward Capriolo <edlinuxg...@gmail.com> wrote: > On Thu, Mar 16, 2017 at 3:10 PM, Jeff Jirsa <jji...@apache.org> wrote: > > > > > > > On 2017-03-16 10:32 (-0700), François Deliège <franc...@instagram.com> > > wrote: > > > > > > To get this started, here is an initial proposal: > > > > > > Principles: > > > > > > 1. Tests always pass. This is the starting point. If we don't care > > about test failures, then we should stop writing tests. A recurring > failing > > test carries no signal and is better deleted. > > > 2. The code is tested. > > > > > > Assuming we can align on these principles, here is a proposal for their > > implementation. > > > > > > Rules: > > > > > > 1. Each new release passes all tests (no flakinesss). > > > 2. If a patch has a failing test (test touching the same code path), > the > > code or test should be fixed prior to being accepted. > > > 3. Bugs fixes should have one test that fails prior to the fix and > > passes after fix. > > > 4. New code should have at least 90% test coverage. > > > > > First I was > > I agree with all of these and hope they become codified and followed. I > > don't know anyone who believes we should be committing code that breaks > > tests - but we should be more strict with requiring green test runs, and > > perhaps more strict with reverting patches that break tests (or cause > them > > to be flakey). > > > > Ed also noted on the user list [0] that certain sections of the code > > itself are difficult to test because of singletons - I agree with the > > suggestion that it's time to revisit CASSANDRA-7837 and CASSANDRA-10283 > > > > Finally, we should also recall Jason's previous notes [1] that the actual > > test infrastructure available is limited - the system provided by > Datastax > > is not generally open to everyone (and not guaranteed to be permanent), > and > > the infrastructure currently available to the ASF is somewhat limited > (much > > slower, at the very least). If we require tests passing (and I agree that > > we should), we need to define how we're going to be testing (or how we're > > going to be sharing test results), because the ASF hardware isn't going > to > > be able to do dozens of dev branch dtest runs per day in its current > form. > > > > 0: https://lists.apache.org/thread.html/f6f3fc6d0ad1bd54a6185ce7bd7a2f > > 6f09759a02352ffc05df92eef6@%3Cuser.cassandra.apache.org%3E > > 1: https://lists.apache.org/thread.html/5fb8f0446ab97644100e4ef987f36e > > 07f44e8dd6d38f5dc81ecb3cdd@%3Cdev.cassandra.apache.org%3E > > > > > > > Ed also noted on the user list [0] that certain sections of the code itself > are difficult to test because of singletons - I agree with the suggestion > that it's time to revisit CASSANDRA-7837 and CASSANDRA-10283 > > Thanks for the shout out! > > I was just looking at a patch about compaction. The patch was to calculate > free space correctly in case X. Compaction is not something that requires > multiple nodes to test. The logic on the surface seems simple: find tables > of similar size and select them and merge them. The reality is it turns out > now to be that way. The coverage itself both branch and line may be very > high, but what the code does not do is directly account for a wide variety > of scenarios. Without direct tests you end up with a mental approximation > of what it does and that varies from person to person and accounts for the > cases that fit in your mind. For example, you personally are only running > LevelDB inspired compaction. > > Being that this this is not a multi-node problem you should be able to re > factor this heavily. Pulling everything out to a static method where all > the parameters are arguments, or inject a lot of mocks in the current code, > and develop some scenario based coverage. > > That is how i typically "rescue" code I take over. I look at the nightmare > and say, "damn i am really afraid to touch this". I construct 8 scenarios > that test green. Then I force some testing into it through careful re > factoring. Now, I probably know -something- about it. Now, you are fairly > free to do a wide ranging refactor, because you at least counted for 8 > scenarios and you put unit test traps so that some rules are enforced. (Or > the person changing the code has to actively REMOVE your tests asserting it > was not or no longer is valid). Later on you (or someone else) __STILL__ > might screw the entire thing up, but at least you can now build forward. > > Anyway that patch on compaction was great and I am sure it improved things. > That being said it did not add any tests :). So it can easily be undone by > the next person who does not understand the specific issue trying to be > addressed. Inline comments almost scream to me 'we need a test' not > everyone believes that. > -- Thank you & Best Regards, --Simon (Qingcun) Zhou
