Re: PRs should always include tests

[Alexander Murmann]

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  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 

Re: PRs should always include tests

[Kirk Lund]

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

Steve
Freeman and Nat Pryce.

On Fri, Dec 29, 2017 at 2:20 PM, Xiaojian Zhou  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  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.
> >>
> >> 

Re: PRs should always include tests

[Alexander Murmann]

Xiaojian, are you describing a situation where we change implementation
because we already have a failing test that somehow got merged in?

On Fri, Dec 29, 2017 at 2:20 PM, Xiaojian Zhou  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  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.
> >>
> >>
> >
>

Re: PRs should always include tests

[Xiaojian Zhou]

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  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.
>>
>>
>

Re: PRs should always include tests

[Udo Kohlmeyer]

+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.

Re: PRs should always include tests

[Alexander Murmann]



On Fri, Dec 29, 2017 at 12:51 PM, Jacob Barrett  wrote:

> +1
>
> > On Dec 29, 2017, at 12:05 PM, 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.
>

Re: PRs should always include tests

[Jacob Barrett]

+1

> On Dec 29, 2017, at 12:05 PM, 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.