Thank you Kirk for the amazing write-up! I want to highlight that the problem of only having one kind of test goes both ways. Frequently we might think that changing something on the unit level is going to resolve a bug, but in reality the real world use case now fails a little later. I think that is a concern best addressed by doing outside-in TDD. You start with a failing test that exercises the code from the user perspective. Those failures should point you to where changes need to be made on the unit level. Otherwise something like this might happen:
On the flip side, as you point out Kirk, there are lots of reasons why you want to have that comes with unit testing a legacy code base. It becomes a forcing function to address our tech debt instead of accumulating more. While it might be at times frustrating that a change that seems on the surface like it should take one hour turns into a much longer ordeal, it's the reality of working with legacy code. As long as we have legacy code it will always slow us down. The only way to get away from that is to pay of the debt and making code unit testable is a great way of doing that and knowing you are moving in the right direction. This ultimately enables us to move fast forever. On Wed, Jan 3, 2018 at 10:59 AM, Kirk Lund <kl...@apache.org> wrote: > This is a very important question and it really showcases the biggest > problem facing Apache Geode in my opinion. > > If existing tests don't fail when you change the code, then that's a sign > of a serious problem with our tests. Most likely the test isn't actually > covering the code and all of its branches or it's missing validation. The > author of such a test may have been manually validating (possibly by > reading System.out.printlns) or didn't understand how or what to assert. > Writing Unit Tests with TDD requires you to see the test fail before > writing or fixing the code to make the test go green. This will prevent you > from ending up with tests that don't have appropriate validation or provide > real coverage. > > This also brings up Unit Test vs Integration Test (here I'm using the term > Integration Test to mean any non-Unit Test including DUnit tests). There is > a lot of confusion about why someone would want Unit Test AND Integration > Test coverage for the same code, but we do need that. I copied some of the > following from various sources but it really spells out the differences > very nicely. > > Unit Tests: > * provide highly transparent white box testing > * promote internal code quality: Maintainability, Flexibility, Portability, > Re-usability, Readability, Testability, and Understandability > * provide test coverage of a single isolated piece of code (aka a class) > and covers all code branches within it including boundary conditions, > failure conditions, corner cases, interactions with mocked dependencies > * should be deterministic, isolated, idempotent, and run very fast (you > should be able to run all Unit Tests within a few seconds) > * should be Repeatable, Consistent, In Memory, Fast > * should test one single concern or use case > * should have only one reason to fail > * must NOT access the file system, communicate with a database, communicate > across the network, prevent running of other unit tests in parallel, > require anything special in the environment > * should ideally cover 100% of the code > > Integration Tests: > * provide black box testing > * promote external code quality: Correctness, Usability, Efficiency, > Reliability, Integrity, Adaptability, Accuracy, and Robustness > * provide test coverage of real classes and components working together > * cannot practically cover all code branches, boundary conditions, or > corner cases > * may access the file system, communicate with a database or communicate > across the network > * should not depend on test hooks in the product code (write a Unit Test to > exercise such failure conditions instead of writing an Integration Test > that requires a test hook) > * should realistically cover much less than 100% of the code (this can be > an ideal goal but it probably cannot be achieved) > * may depend on environment and external systems, be much slower than Unit > Tests and may test multiple things in one test case > > If you read between the lines you'll see a strong correlation between > developing-with-high-level-or-Integration-Tests-only (ie > low-to-no-coverage > with Unit Tests) resulting in code that has high technical debt and becomes > very difficult to maintain or add to. > > The best discussion about why you should have both types of tests for the > same code is in the book Growing Object-Oriented Software, Guided by Tests > [1] by Steve Freeman and Nat Pryce. > > If you have code that you cannot write a Unit Test for then you have > classic untestable legacy code that you need to refactor so that it can be > properly unit tested and maintained. This means we have technical debt that > we must pay down and we should all be contributing to this effort. The best > way to fit this in and create a reasonable balance of fixing tech debt vs > new work is to adopt the discipline of ALWAYS writing a Unit Test for any > code you touch that doesn't already have coverage from a Unit Test > (regardless of higher level test coverage). > > Changing a class to be testable usually involves: > * changing or adding a constructor to pass in dependencies (so you can pass > mocks in) > * changing the class to depend on interfaces instead of impls > * eliminating non-constant static variables and reducing (or eliminating) > usage of statics in general > > See also: > * http://wiki.c2.com/?InternalAndExternalQuality > * > https://meekrosoft.wordpress.com/2010/10/31/internal-and- > external-software-quality/ > > [1] Growing Object-Oriented Software, Guided by Tests > <http://www.amazon.com/gp/product/0321503627?ie=UTF8&; > tag=meekrosoft-20&linkCode=as2&camp=1789&creative=390957& > creativeASIN=0321503627> > Steve > Freeman and Nat Pryce. > > On Fri, Dec 29, 2017 at 2:20 PM, Xiaojian Zhou <gz...@pivotal.io> wrote: > > > How about the code change is already covered by existing tests? > > > > Not to reduce test coverage seems a more reasonable standard. > > > > On Fri, Dec 29, 2017 at 2:07 PM, Udo Kohlmeyer <u...@apache.org> wrote: > > > > > +1 > > > > > > > > > > > > On 12/29/17 12:05, Kirk Lund wrote: > > > > > >> I think we all need to be very consistent in requiring tests with all > > PRs. > > >> This goes for committer as well as non-committer contributions. > > >> > > >> A test would both confirm the existence of the bug in the first place > > and > > >> then confirm the fix. Without such a test, any developer could come > > along > > >> later, modify the code in question and break it without ever realizing > > it. > > >> A test would protect the behavior that was fixed or introduced. > > >> > > >> Also if we are not consistent in requiring tests for all > contributions, > > >> then contributors will learn to pick and choose which reviewers to > > listen > > >> to and which ones to ignore. > > >> > > >> I for one do not want to waste my time reviewing and requesting > changes > > >> only to be ignored and have said PR be committed without the > (justified) > > >> changes I've requested. > > >> > > >> > > > > > >
