bring Lib/copy.py to 100% coverage #55781
Comments
|
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. |
|
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 <[email protected]>:
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. |
|
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 <[email protected]> wrote:
"#pragma NO COVER" is recognized by stdlib trace module, so it is not
Agree, but running every line of code is a good first step. For |
|
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?
You could change that to assertIsInstance.
|
|
Éric Araujo <[email protected]> writes:
Because it is impossible for this code to be called in Python 3 -
I will do so on Monday to make a final form of the patch, along with any
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 <[email protected]> writes:
I had initially wanted to make a comment, but feared the objection that |
|
Nick Coghlan <[email protected]> writes:
I just sat down to review this issue, and, looking at test_copy3.patch, Finally, Éric wants me to replace this:
with self.assertIsInstance(). But surely the question is not whether So I think that test_copy3.patch already includes all of the valid |
|
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_immutableWhat 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): 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:
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. |
|
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': |
|
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? |
berkerpeksag
added
stdlib
|
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 |
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:
The text was updated successfully, but these errors were encountered: