classification
Title: Increase logging/__init__.py coverage to 97%
Type: behavior Stage: patch review
Components: Tests Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vinay.sajip Nosy List: brett.cannon, drakeol, vinay.sajip
Priority: normal Keywords: patch

Created on 2011-02-26 11:18 by drakeol, last changed 2011-04-28 11:13 by vinay.sajip. This issue is now closed.

Files
File name Uploaded Description Edit
test_logging.diff drakeol, 2011-02-26 11:17 review
Messages (9)
msg129530 - (view) Author: Oliver Drake (drakeol) Date: 2011-02-26 11:17
Purely a modification to test_logging.py with the focus being to increase coverage. coverage.py now measures 97% (when running test_logging.py by itself). I'm not sure if I've followed py-dev unit test conventions exactly, I've created quite a few new test case classes, going by the model of having one unittest test case class for each class defined in the module under test. Comments welcome :)
msg129560 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-02-26 15:25
Thanks for doing this, I appreciate the effort you've put into it. My comments:

1. Each test class ought to be independent, but they aren't - for example, if I comment out all of your test classes other than LoggerTest, it fails.
2. I try and avoid modifying the handler list across tests. Rather than running test_logging.py by itself, it should run cleanly when run using

pythonX.Y regrtest.py test_logging test_logging

in the Lib/test folder. (That's right, I invoke it twice because that sometimes flushes out bugs which don't show up with a single invocation.)
3. I'm not if it's worth it to monkey-patch stuff (such as the _log method in test_log_invalidLevel), or even patching io.StringIO in test_findCaller. In general more coverage is good, but I also favour being pragmatic about tests. IMO, testing at too low a level can incur unnecessary overhead in re-implementing tests if some internals are refactored. Some of your tests do fall into that category, e.g. checking that the "parent" attribute is polled is an implementation detail, though checking hasHandlers() isn't (in test_hasHandlers_breakOnPropagateFalse). Another example: that logging.disable sets logging.root.manager.disable is an implementation detail, and so not worth checking as much as checking the *effect* of the disable call on logging output.

ISTM other tests in the stdlib test suite do not go to the low level that yours do - essentially mocking parts of the system. Although undoubtedly I should incorporate some of your tests to fill in gaps in coverage, I am not sure I want to just accept the patch as it is, for the reasons I have given above.  (Notwithstanding that, please understand that I still appreciate the effort you've put into this somewhat unglamorous activity.)

As a matter of interest, what was the coverage percentage *before* you made your changes?
msg129593 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-26 20:57
Just to give an opinion (which can be ignored), I see no issue with mocking if done carefully and properly. It should just be kept to a minimum (and typically be privately exposing certain things that can be overridden).
msg129606 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-02-26 23:57
@Brett: I've no religious/dogmatic objection to mocking, either, so I agree with your comment. I merely observed that it's not generally implemented in the Python test suite (not that I've made an exhaustive study of it, so I'm open to being corrected on this point). Also, I don't see much value in some of the low-level tests in the patch. For example: logging.disable is defined as

def disable(level):
    """
    Disable all logging calls less severe than 'level'.
    """
    root.manager.disable = level

The test for it is:

    def test_disable(self):
        disableBackup = logging.root.manager.disable
        logging.disable(10)
        self.assertEqual(logging.root.manager.disable, 10)
        logging.root.manager.disable = disableBackup

which is really just testing at a low level - that the attribute was set - but not more usefully, at a higher level, the practical effect of the disable call.

I appreciate the value that tests bring, but I'm just being a bit wary of letting the tail wag the dog too much in the interests of higher coverage ;-) So, I suppose my test case preferences tend more towards an acceptance test style rather than a "pure" unit test style.
msg129607 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-27 00:45
OK, but acceptance tests do not need to not try to get higher test coverage. For instance, for testing disable() simply using it and making sure the outcome is as expected also works.

I can understand wanting to avoid some low-level whitebox testing, but I don't think that precludes getting better coverage results.
msg129615 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-02-27 08:04
> OK, but acceptance tests do not need to not try to get higher  test coverage. 
>For instance, for testing disable() simply using it and making  sure the outcome 
>is as expected also works.

> 
> I can understand wanting to  avoid some low-level whitebox testing, but I don't 
>think that precludes getting  better coverage  results.

Well then, it sounds like we're on the same page. I'm not arguing against better 
coverage, just against the low-level whitebox testing elements of Oliver's 
patch. He did welcome comments, after all :-)
msg129648 - (view) Author: Oliver Drake (drakeol) Date: 2011-02-27 19:42
Thanks for the comments and prompt response :) I think I misunderstood the nature and level of these unit tests. I will fix the specific issues you mentioned, and either cut or modify the less useful/too low level tests (e.g. disable). In general I will change my approach to be more high level. I will steer away from testing the implementation line by line, but I believe there should be unit tests that enforce the API that is published to the user - i.e. one unit test for every class, method and function, testing inputs, outputs, exception handling and general behavior. So if a developer changes the API or the general behavior of a function he/she should have to change the documentation and the appropriate unit test - or do we want to avoid this type of testing completely?
msg129650 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-27 20:33
Testing the documented API is definitely wanted, Oliver. Any change in behaviour needs to be detected to ensure there is not backwards-compatibility regressions without it being intentional.
msg134668 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-04-28 11:13
Though I did not use this patch verbatim, thank you, Oliver, for the impetus to improve coverage. Now, coverage of the logging package is:

logging/__init__.py 99% (97%)
logging/config.py 89% (85%)
logging/handlers.py 65% (60%)

where the brackets include branch coverage measurements. Not too shabby, so I'll close this issue now: I think future coverage increases will need to focus on the handlers.
History
Date User Action Args
2011-04-28 11:13:20vinay.sajipsetstatus: open -> closed
resolution: fixed
messages: + msg134668
2011-02-27 20:33:38brett.cannonsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129650
2011-02-27 19:42:21drakeolsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129648
2011-02-27 08:04:21vinay.sajipsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129615
2011-02-27 00:45:16brett.cannonsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129607
2011-02-26 23:57:10vinay.sajipsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129606
2011-02-26 20:57:10brett.cannonsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129593
2011-02-26 15:25:18vinay.sajipsetnosy: brett.cannon, vinay.sajip, drakeol
messages: + msg129560
2011-02-26 11:41:17pitrousetversions: + Python 3.3
nosy: + brett.cannon, vinay.sajip

assignee: vinay.sajip
type: behavior
stage: patch review
2011-02-26 11:18:01drakeolcreate