[python-committers] Codecov and PR

[Terry Reedy]

On 4/26/2017 1:45 PM, Brett Cannon wrote:
> 
> On Tue, 25 Apr 2017 at 17:00 Terry Reedy <tjreedy at udel.edu 
> <mailto:tjreedy at udel.edu>> wrote:

>     While I use code coverage to improve automated unittesting, I am opposed
>     to turning a usable but limited and sometime faulty tool into a blind
>     robotic master that blocks improvements.  The prospect of this being
>     done has discouraged me from learning the new system.  (More on 'faulty
>     tool' later.)
> 
> It should be stated that code coverage is not a blocking status check 
> for merging from our perspective (the **only** required check is that 
> Travis pass with it's test run).

I have the impression that at one time you hoped to make it blocking. 
If that was wrong, I apologize for misunderstanding.  If you have 
changed your mind, then I am happy.

I am otherwise in favor of both the measurement and report of coverage 
being improved.

>     The temptation to write artificial tests to satisfy an artificial goal
>     is real.  Doing so can eat valuable time better used for something else.
>        For instance:
> 
>           def meth(self, arg):
>               mod.inst.meth(arg, True, ob=self, kw='cut')
> 
>     Mocking mod.class.meth, calling meth, and checking that the mock is
>     called will satisfy the robot, but does not contribute much to the goal
>     of providing a language that people can use to solve problems.

> My assumption is that there will be a test that meth() does the right 
> thing, mock or not. If we provide an API there should be some test for 
> it; using a mock or something else to do the test is an implementation 
> detail.

My impression is that default mocks have a generic signature, so that 
merely checking that the mock is called will not catch an invalid call. 
I presume that one can do better with mocks, and I have with custom 
mocks I have written, but my point above was that coverage does not care.

>      >> If it's not important enough to require tests >> it's not
>     important enough to be in Python.  ;)
> 
>     Modules in the test package are mostly not tested. ;)
> 
> 
> :) But they are at least executed which is what we're really measuring 
> here and I think all Ethan and I are advocating for.

I thought Ethan was advocating for more -- a specific unittest for each 
line.

> E.g. I don't expect 
> test_importlib to be directly responsible for exercising all code in 
> importlib, just that Python's entire test suite exercise importlib as 
> much as possible as a whole.

The advantage for importlib in this respect is that import statements 
cannot be mocked; only the objects imported, after importlib is finished.

There is lots of interaction between idlelib modules, but I would still 
like direct tests of each idlelib.xyz with a test_xyz.py.  Three years 
ago, there was no test.test_idle.  There now is, and it runs 35 
idlelib.idle_test.text_* files.  (There are 60 idlelib modules.)

> The problem I have with just doing manual testing for things that can 
> easily be covered by a unit test -- directly or indirectly -- is there 
> are technically 85 people who can change CPython today. That means 
> there's potentially 85 different people who can screw up the code ;) . 

At the moment, I am the only one pushing idlelib patches, except when it 
gets included in one of Serhiy's multi-module refactoring patches (and 
he always nosies me).  That skews my view a bit.  However, with most of 
the critical issues fixed, I am anxious to automate what I can of what I 
now do by hand.

> Making sure code is exercised somehow by tests at least minimizes how 
> badly someone like me might mess something thanks to me not realizing I 
> broke the code.

I had not thought about the issue that way.  I should add a test_module 
for each remaining module, import the module, and at least create an 
instance of every tkinter widget defined therein, and see what other 
classes could be easily instantiated and what functions easily run.
>     Some practical issues with coverage and CodeCov:

>     2. Some statements are only intended to run on certain systems, making
>     100% coverage impossible unless one carefully puts all system-specific
>     code in "if system == 'xyz'" statements and uses system-specific
>     .coveragerc files to exclude code for 'other' systems.

> True. We could have a discussion as to whether we want to use 
> Coverage.py's pragmas ... I'm sure we could discuss things with Ned 
> Batchelder if we needed some functionality in coverage.py for our needs).

Let's skip this for now.

>     3. Some tests required extended resources.  Statements that are only
>     covered by such tests will be seen as uncovered when coverage is run on
>     a system lacking the resources.  As far as I know, all non-Windows
>     buildbots and CodeCov are run on systems lacking the 'gui' resource.  So
>     patches to gui code will be seen as uncovered.
> 
> I view 100% coverage as aspirational, not attainable. But if we want an 
> attainable goal, what should we aim for? We're at 83.44% now

On what system?  I suspect that Tkinter, ttk, turtle, and IDLE 
GUI-dependent tests make at least a 2% difference on GUI Windows versus 
no-GUI *nix.

   we could
> say that 80% is something we never want to drop below and be done with 
> it. We could up it to 85% or 90% in recognizing that there is more 
> testing to do but that we will never cover all Python code (all of this 
> is configurable in Codecov, hence why I'm asking).

Since I think we actually are at 85%, and certainly will be when I add 
minimal easy tests for the rest of IDLELIB, I think 90% would be a 
reasonable goal.

One way to increase coverage is to push a bit harder on fulfilling the 
'test needed' stage.  Theoretically, every substantive 
(behavior-changing) patch should start with a test that fails.  Since 
PRs are separate from the main repository and can be patched separately, 
a PR could start with a test that should immediately fail but should 
pass before merging.  It would be nice if the test runner knew to only 
run the new test and not the entire suite.  It would be even nicer if it 
know that initial failure is success.  Is there at least a 'New Test' 
label on PRs?

>     4. As I explained in a post on the core-workflow list, IDLE needs the
>     following added to the 'exclude_lines' item of .coveragerc.
>           .*# htest #
>           if not _utest:

These additions would remove, I think, at least 400 lines from the 
uncovered category.  Both only occur in idlelib.

--
Terry Jan Reedy