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

1. I prefer that we don't have pragma statements sprinkled over the stdlib.
2. You can use assertIs() instead of assertTrue(x is y) (Feel free to change the lot)
3. 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 issue2506 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.

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

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

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

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

Éric Araujo <report@bugs.python.org> writes:

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

Because it is impossible for this code to be called in Python 3 -
"object" now itself supplies __reduce_ex__, so the fall-through to look
for plain "__reduce__" does not occur here.  This default __reduce_ex__
then calls any user-defined __reduce__, if present.

>>         self.assertTrue(issubclass(copy.Error, Exception))
> You could change that to assertIsInstance.

I will do so on Monday to make a final form of the patch, along with any
other suggestions that come in!

>> -def _test():
> I assume that all cases that were tested in that function are covered
> by unit tests.

Yes!

Regarding "__reduce__", other readers will have the same question Éric did, so that point should definitely go in a comment after the "__reduce_ex__" check.

Nick Coghlan <report@bugs.python.org> writes:

> Nick Coghlan <ncoghlan@gmail.com> added the comment:
>
> Regarding "__reduce__", other readers will have the same question Éric
> did, so that point should definitely go in a comment after the
> "__reduce_ex__" check.

I had initially wanted to make a comment, but feared the objection that
a comment would eventually fall out of sync with the implementation of
object.__reduce_ex__ over the years (just as copy.py currently has all
sorts of cruft that is no longer applicable).  But I think that you are
right that a comment that's at least true today is better than no
comment at all; so I will add one on Monday.

Nick Coghlan <report@bugs.python.org> writes:

> Regarding "__reduce__", other readers will have the same question Éric
> did, so that point should definitely go in a comment after the
> "__reduce_ex__" check.

I just sat down to review this issue, and, looking at test_copy3.patch,
isn't there already a comment next to each __reduce_ex__ check that
reminds the reader that object.__reduce_ex__ will itself call
__reduce__?  Does the comment just need to be more elaborate or
something?

Finally, Éric wants me to replace this:

>         self.assertTrue(issubclass(copy.Error, Exception))

with self.assertIsInstance().  But surely the question is not whether
copy.Error is an *instance* of Exception?  They are both instances of
*type*, right?  What I would need is something like assertIsSubclass or
assertInheritsFrom, neither of which exists.

So I think that test_copy3.patch already includes all of the valid
improvements on the table; if I'm missing one, just point it out and
I'll fix it!

The suggestion about assertIsInstance was a mistake, I misread issubclass in the original code.

Thanks for the updated patch.

-t = getattr(types, "CodeType", None)
-if t is not None:
-    d[t] = _copy_immutable
+d[types.CodeType] = _copy_immutable

What was the use case for this again?  The defunct restricted mode, another VM or something else?  IOW, are we sure this change isn’t going to break something?

+    def _copy_with_copy_method(x):
+        return x.copy()
+    d[PyStringMap] = _copy_with_copy_method  # for Jython

Could even just be d[PyStringMap] = PyStringMap.copy

Éric, I think your points are good ones. (And, as I return to this patch after three months, I should thank the PSF for sponsoring the CPython sprint here at PyOhio, and creating this opportunity for me to continue trying to land this patch!) I am attaching a fourth version of the patch. It incorporates your two suggestions, Éric. It also applies cleanly once against today's trunk; besides the usual line number changes as code has come and gone, I am happy to see that my change of an "assertTrue" for an "assertIs" in the test suite has already taken place thanks to another patch in the meantime.

Hi Brandon, I really like to see your patch applied, let's see what I can do (I also added Ezio in the loop).

I think you only addressed half of msg132140 : could you please have a look at the first Éric's question?

Also, still Éric made a comment on rietveld (you'd access to it clicking on 'review' next to your patch) so it would be nice if you can reply to that too.

About the ~100% coverage, it's probably me but I can't get more then 67%:

$ ./python -m coverage run --pylib --source=copy Lib/test/regrtest.py test_copy
[1/1] test_copy
1 test OK.
$ ./python -m coverage report | grep copy
Lib/copy     182     60    67%

How did you compute the coverage?

ISTM that the patch is trying to do too many things at once:
1) increase the test coverage, possibly fixing some bugs discovered while doing so;
2) refactor the tests to use the correct assert methods;
3) get rid of old code, and do some refactoring in copy.py;

I'm not sure any of the changes in copy.py is necessary to make the test suite pass, even after the additions you included in the patch (I haven't tested though).  If this is the case, the refactoring/cleanup of copy.py should IMHO be committed separately.
For the tests it's probably fine to commit both the additions and the refactoring together (i.e., it's not worth wasting time splitting the patch).

After a quick chat with Ezio, we tried to revert the changes to copy.py while keeping the ones on test, and the test suite passes.

The next steps would probably be to just commit the diff for test_copy.py and see if the changes on copy.py are really worth.

Nick, since this issue is assigned to you, what do you want to do? would you like me to handle the test-commit part and still be assigned to you for the remaining part?

Ezio and Sandro, thank you very much for your attention to this issue, and for helping me split it into manageable chunks! To answer the question about why "coverage" does not show as high a total as it ought: it's because coverage normally can't see the outer, global scope execution of modules that are already imported by the time "coverage" itself can take control and install a tracer. I have another patch outstanding that fixes this — we are still working on it, but the code works fine — if you want to run "coverage" and see a more accurate number: http://bugs.python.org/issue11561

I'd assigned this to myself since I was discussing it with Brandon when he was working on it at the PyCon sprints. Since I certainly don't want to block anyone else getting to it, I'm unassigning it - feel free to take it forward :)

IIRC, the copy.py changes were things Brandon and I discussed at the sprints as cases where they were legacy code that was no longer needed in Py3k, so it made more sense to just delete them rather than add tests to cover them. Definitely makes sense to split those changes out into a separate patch, though (easier to revert if we later discover the code isn't as useless as we think it is).

New changeset 74e79b2c114a by Sandro Tosi in branch 'default':
#11572: improvements to copy module tests along with removal of old test suite
http://hg.python.org/cpython/rev/74e79b2c114a

Brandon, thanks for your work on this patch! I've just committed the unittests update+removal of _test() part.

For the remaining part, I see that Nick and you worked on it during a sprint, so I'm quite sure it's fine, but nonetheless it would be awesome if you can fill in the missing bits.

re coverage: I saw the other issue, but I didn't connect the dots, thanks for doing it for me :)

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 #502085)
-            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.

New changeset 2708578736d1aa15685495e9b94b827a8e185a8c by Berker Peksag in branch 'master':
bpo-11572: Make minor improvements to copy module (GH-8208)
https://github.com/python/cpython/commit/2708578736d1aa15685495e9b94b827a8e185a8c

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