Repository navigation
bring Lib/copy.py to 100% coverage #55781
Description
Activity
brandon-rhodes commented
on Mar 16, 2011 brandon-rhodesmannequinMannequinAuthorThe attached patch will bring Lib/copy.py to 100% test coverage.
A bug in "coverage" results in its only reporting 99% at the moment; see coverage issue #122 on bitbucket:
https://bitbucket.org/ned/coveragepy/issue/122/for-else-always-reports-missing-branch
This patch makes several minor improvements to "copy": when doing getattr lookups with a default of "None", it now uses an "is" comparison against None which is both faster and more correct; several special cases have been removed since Python 3 always has "CodeType" available; and an ancient obsolete test suite that had been appended to copy.py in ancient times has been removed.
- I prefer that we don't have pragma statements sprinkled over the stdlib.
- You can use assertIs() instead of assertTrue(x is y) (Feel free to change the lot)
- Along the way you could also change those "raise support.TestFailed" over to calling the TestCase.fail method.
A bug in "coverage" results in its only reporting 99% at the moment
It was concluded during discussion of bpo-2506 that this is not a bug. At least not a bug in "coverage" or the trace module. At most this is a peephole optimization issue or rather a missing feature to turn off peephole optimization in coverage runs.
Alexander: the coverage problem in this case has to do with it incorrectly handling an "else:" clause on a loop (it doesn't adjust the expected target for an empty sequence to be the body of the else clause)
Benjamin: the pragma question is probably worth bringing up on python-dev. In the long run, it would be best to merge coverage data from the test suites of all the major implementations and platforms, but in the meantime we need a way to mark when modules are "done" from the point of view of the CPython test suite.
2011/3/16 Nick Coghlan <report@bugs.python.org>:
Nick Coghlan <ncoghlan@gmail.com> added the comment:
Alexander: the coverage problem in this case has to do with it incorrectly handling an "else:" clause on a loop (it doesn't adjust the expected target for an empty sequence to be the body of the else clause)
Benjamin: the pragma question is probably worth bringing up on python-dev. In the long run, it would be best to merge coverage data from the test suites of all the major implementations and platforms, but in the meantime we need a way to mark when modules are "done" from the point of view of the CPython test suite.
There's really no precedent for it.
brandon-rhodes commented
on Mar 16, 2011 brandon-rhodesmannequinMannequinAuthorBenjamin, thanks for the pointers! The attached patch now uses assertIs() and assertIsNot(), and calls self.fail() instead of using the exception from "support".
In the future I would like some way to determine when test coverage is fully achieved, so that I do not come back to this module every few months and have to re-discover why it is not 100%. But for the moment I have indeed removed the pragmas, pending a better approach!
Some code is removed by your patch, for example a check that prevented an error for builds that don’t include complex. If this type of builds still exist, the code should be compatible.
Conversely, the code mentions unicode in some places, but it’s gone now.
brandon-rhodes commented
on Mar 17, 2011 brandon-rhodesmannequinMannequinAuthorÉric, the Makefile in Python trunk seems to include Objects/complexobject.o in the build unilaterally without any way to turn it off. What is leading you to believe that Python 3 can conditionally turn the "complex" type off during a build?
I do not understand your question about Unicode — could you reference the line number in the patch file that is worrying you?
+1 for not having pragma statements in the stdlib, especially when they target a third-party tool few of us know and use.
Beside, while coverage measurements are useful, trusting them that actual coverage is complete is IMO a bit foolish. It's not just about running every line of code.brandon-rhodes commented
on Mar 18, 2011 brandon-rhodesmannequinMannequinAuthorAntoine, neither this issue, nor either version of my patch, was intended to assert that 100% test coverage indicates that a test of tests are complete. If you will point out where in the text this is implied, I will correct it. Thanks!
On Thu, Mar 17, 2011 at 8:03 PM, Antoine Pitrou <report@bugs.python.org> wrote:
..+1 for not having pragma statements in the stdlib, especially when they target a third-party tool few of us know and use.
"#pragma NO COVER" is recognized by stdlib trace module, so it is not
specific to a third-party tool. I don't like #pragma myself. This is
very "unpythonic" choice. I would be much happier if it was simply '#
NO COVER' or something even less conspicuous. I do believe, however
that some mechanism should be used to prevent trace from highlighting
spurious lines as lacking coverage.Beside, while coverage measurements are useful, trusting them that actual coverage is complete is IMO a bit foolish.
It's not just about running every line of code.Agree, but running every line of code is a good first step. For
example, it is *very* useful for identifying lack of coverage for
error conditions.A little research has found that building without complex is not possible anymore, so you’re good: http://bugs.python.org/issue7147
Regarding “unicode”, see line 112.
brandon-rhodes commented
on Mar 18, 2011 brandon-rhodesmannequinMannequinAuthorÉric, after checking line 112 of the two patches and then of the new file, I figured out that you meant line 112 of the old file — and, yes, that test can go away too since in python3 "complex" always exists and "unicode" never exists. A further improved patch (#3) is attached.
Nice cleanup.
- reductor = getattr(x, "__reduce__", None) - if reductor: - rv = reductor() - else: - raise Error("un(shallow)copyable object of type %s" % cls) + raise Error("un(shallow)copyable object of type %s" % cls) Why change this?
self.assertTrue(issubclass(copy.Error, Exception))You could change that to assertIsInstance.
-def _test():
I assume that all cases that were tested in that function are covered by unit tests.16 remaining items
JFTR, the same kind of check of __reduce_ex__ and then falling back on __reduce__ is prenset in pickle too: maybe it's worth align them?
Is anything left to do with this issue? Many changes were made in the copy module and tests last year.
All changes to Lib/test/test_copy.py have already been committed.
Perhaps the following hunks from the latest patch can still be useful:
-try: - d[types.CodeType] = _deepcopy_atomic -except AttributeError: - pass +d[types.CodeType] = _deepcopy_atomic
def _deepcopy_tuple(x, memo): + if not x: + return x
- try: - issc = issubclass(cls, type) - except TypeError: # cls is not a class (old Boost; see SF python/cpython#35903) - issc = 0 - if issc: + if issubclass(cls, type):
Agree. The latter simplification can be applied to pickle.py.
copy and pickle use the same protocol. The copy module shouldn't be changed independently, the changes should be synchronised with both implementations of the pickle module.
Thanks, Brandon.
I realize now that calling self.fail at https://hg.python.org/cpython/rev/74e79b2c114a#l2.20 is a problem: self is an instance of the C class, not the TestCase instance.
(The line is unreachable anyway so this doesn’t matter a lot. In other projects I’d use something like
__reduce__ = raiser(AssertionError)to avoid this.)
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields: