Performance consideration is a valid concern. When I say "difficult to write unit test cases", I mean sometimes you need to make method/variable package private, or create a package private constructor so that you can inject some internal states, etc. This is more like "annoying" if it's not "difficult". But I agree that good coding practice leads to easier unit testing.
When we talk about code coverage for new code, should we encourage people to contribute unit test cases for existing code? On Thu, Mar 16, 2017 at 2:27 PM, DuyHai Doan <doanduy...@gmail.com> wrote: > "Otherwise it'll be difficult to write unit test cases." > > Having to pull in dependency injection framework to make unit testing > easier is generally a sign of code design issue. > > With constructor injection & other techniques, there is more than enough to > unit test your code without having to pull external libs > > On Thu, Mar 16, 2017 at 10:18 PM, Jason Brown <jasedbr...@gmail.com> > wrote: > > > >> do we have plan to integrate with a dependency injection framework? > > > > No, we (the maintainers) have been pretty much against more frameworks > due > > to performance reasons, overhead, and dependency management problems. > > > > On Thu, Mar 16, 2017 at 2:04 PM, Qingcun Zhou <zhouqing...@gmail.com> > > wrote: > > > > > 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 > > > > > > -- Thank you & Best Regards, --Simon (Qingcun) Zhou
