classification
Title: functools test coverage
Type: behavior Stage: committed/rejected
Components: Tests Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Thorney, eric.araujo, eric.snow, ezio.melotti, jackdied, ncoghlan, pitrou, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2011-06-28 10:07 by Thorney, last changed 2012-11-14 07:16 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
functools.diff Thorney, 2011-06-28 10:07 Diff against ddb8a29a6bc5 review
functools2.diff Thorney, 2011-07-30 06:08 Diff against 2428c239d848 review
12428.patch Thorney, 2012-04-12 03:32 Diff against bd353f12c007 review
issue12428.patch Thorney, 2012-07-24 06:00 Diff against 78263:00db71b3c5bd review
functools.patch Thorney, 2012-08-05 07:45 Diff against 5284e65e865b review
Messages (19)
msg139353 - (view) Author: Brian Thorne (Thorney) Date: 2011-06-28 10:07
The test coverage for functools was down around ~60%, this is a patch to bring that up to ~98%.

Made two changes to the Lib/functools.py file itself:

1) Moved the Python implementation of partial into Lib/functools.py from Lib/test/test_functools.py which gets imported over the same as the Python implementation of cmp_to_key.

2) In order to allow the blocking of _functools, I grouped and moved the import functions from of _functools to the end of the file.

In the test_functools.py file:

3) Added two new tests to TestLRU.

4) Add testing of Python implementation of cmp_to_key. I copied how test_warnings.py tests C and Python implementations of the same function.

5) Made the importing of functools itself far less clear
msg139358 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-06-28 12:16
Raymond, do we care whether or not the pure Python version of functools.partial supports inheritance and instance testing?

The constructor is technically documented as returning a "partial object" rather than a simple staticmethod instance with additional attributes.

My own preference leans towards keeping the closure based implementation due to its simplicity, which is what makes it particularly useful as a cross-check on the C implementation.
msg139359 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-28 12:24
> Raymond, do we care whether or not the 
> pure Python version of functools.partial 
> supports inheritance and instance testing?

We don't care.  The docs make very few
guarantees beyond the core functionality.
Everything else is an implementation detail.
msg141425 - (view) Author: Brian Thorne (Thorney) Date: 2011-07-30 06:08
Cheers for the comments Eric. I've modified the patch accordingly.
msg149082 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-09 09:53
Brian's patch looks ok to me. There's a missing newline (or two) just before test_main() but that can easily be fixed on commit.
msg149105 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-12-09 15:38
Ezio and I made further minor comments that can be handled by the person doing the commit; I’d like to do it.
msg153778 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-20 12:44
Just noticed one minor nit with the patch: the pure Python version of functools.partial should support "func" as a keyword argument that is passed to the underlying object. The trick is to declare a positional only argument like this:

def f(*args, **kwds):
    first, *args = args
    # etc...
msg153779 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-20 12:50
Also, the closure based implementation should be decorated with @staticmethod (see http://bugs.python.org/issue11704) and the tests updated accordingly.
msg158101 - (view) Author: Brian Thorne (Thorney) Date: 2012-04-12 03:32
I've updated the patch to address the comments here and in the code review.

I added more cross testing of the pure Python implementation of partial - as you pointed out inheritance wasn't supported so I changed from the simple closure to a class implementation.

Instead of skipping repr tests for the pure Python implementation could we not just implement it? I did skip the pickle test for the Python implementation though.

Nick, I wasn't sure how to decorate the partial object as a staticmethod so I don't think this patch addresses issue 11704.

Also I didn't understand why Lock was being imported from _thread instead of thread. Since coverage went to 100% and the tests continue to all pass when I changed this.
msg166254 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-23 23:44
Why does the pure Python version of partial have to be so complicated?
I don't think the __class__, __setattr__ and __delattr__ are useful. As Raymond said, only the core, documented functionality needs to be preserved, not implementation details.

Something else:

-    from _thread import allocate_lock as Lock
+    from thread import allocate_lock as Lock

The module is named _thread in 3.x, so this shouldn't have been changed (but admittedly it's thread in 2.x).
msg166264 - (view) Author: Brian Thorne (Thorney) Date: 2012-07-24 06:00
Thanks Antoine, the __class__ attribute wasn't useful, I've removed that. Overriding the __setattr__ and __delattr__ gives consistent behaviour with the both versions - allowing the unittest reuse. Also I've changed thread back to _thread.

Isn't more compatibility between the Python and C implementations desired? Is it an aim to document more of the functionality? An earlier version of this patch had a closure implementation of partial; I'm happy to revert to that if simplicity is preferred to compatibility?

Should the caching decorators be tested from multiple threads?
msg166318 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-24 17:33
Le mardi 24 juillet 2012 à 06:00 +0000, Brian Thorne a écrit :
> Isn't more compatibility between the Python and C implementations
> desired?

IMHO, not when compatibility regards obscure details such as whether
setting an attribute is allowed or not. I don't know why this was
codified in the unit tests in the first place, perhaps Nick can shed
some light.

> Should the caching decorators be tested from multiple threads?

Why not, if there's an easy way to do so.
msg167473 - (view) Author: Brian Thorne (Thorney) Date: 2012-08-05 07:45
Back to a simpler closure implementation of partial and skip the repr test for python implementation.
msg175523 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-13 20:37
New changeset fcfaca024160 by Antoine Pitrou in branch 'default':
Issue #12428: Add a pure Python implementation of functools.partial().
http://hg.python.org/cpython/rev/fcfaca024160
msg175524 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-13 20:37
Sorry for the delay. I have now committed the patch to 3.4 (default). Thank you!
msg175529 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-11-13 23:48
The following from the changeset left me with questions:

-from _functools import partial, reduce
+try:
+    from _functools import reduce
+except ImportError:
+    pass

* Why the try block when there wasn't one before?
* Should reduce be added to __all__ only conditionally?
* Should the pure Python partial only be used if _functools.partial is not available?
* Should _functools.partial be removed?
msg175530 - (view) Author: Brian Thorne (Thorney) Date: 2012-11-14 00:14
> * Why the try block when there wasn't one before?
> * Should reduce be added to __all__ only conditionally?

My mistake, the try block should have just covered the import of partial - that is after all the exceptional circumstance we can deal with by using the pure python implementation.

Possibly reduce could be handled in a similar way with a fallback python implementation? Otherwise your suggestion of conditionally adding it to __all__ makes sense to me.

> * Should the pure Python partial only be used if _functools.partial is not available?
> * Should _functools.partial be removed?

What are the main considerations to properly answer these last questions? Performance comparison between the implementations, maintainability?
msg175532 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-11-14 03:51
> Possibly reduce could be handled in a similar way with a fallback python
> implementation? Otherwise your suggestion of conditionally adding it to __all__
> makes sense to me.

In the meantime I'd expect the import of _functools.reduce to not be wrapped in a try block.  Does that have an impact on coverage?

>> * Should the pure Python partial only be used if _functools.partial is not available?
>> * Should _functools.partial be removed?
>
> What are the main considerations to properly answer these last questions? Performance
> comparison between the implementations, maintainability?

Sorry, the first time through I missed the part of the patch that tries to import _functools.partial _after_ the pure Python version is defined.
msg175543 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-14 07:16
> > Possibly reduce could be handled in a similar way with a fallback python
> > implementation? Otherwise your suggestion of conditionally adding it to __all__
> > makes sense to me.
> 
> In the meantime I'd expect the import of _functools.reduce to not be
> wrapped in a try block.  Does that have an impact on coverage?

I tried to remove the try block, but when making the import
unconditional the tests fail with an ImportError (because reduce doesn't
have a pure Python implementation). I haven't investigated further,
since the try block doesn't look like a real issue to me.
History
Date User Action Args
2012-11-14 07:16:36pitrousetmessages: + msg175543
2012-11-14 03:51:05eric.snowsetmessages: + msg175532
2012-11-14 00:14:29Thorneysetmessages: + msg175530
2012-11-13 23:48:19eric.snowsetnosy: + eric.snow
messages: + msg175529
2012-11-13 20:37:48pitrousetstatus: open -> closed
versions: + Python 3.4, - Python 3.3
messages: + msg175524

resolution: fixed
stage: commit review -> committed/rejected
2012-11-13 20:37:09python-devsetnosy: + python-dev
messages: + msg175523
2012-08-05 07:45:12Thorneysetfiles: + functools.patch

messages: + msg167473
2012-07-24 17:33:29pitrousetmessages: + msg166318
2012-07-24 06:00:25Thorneysetfiles: + issue12428.patch

messages: + msg166264
2012-07-23 23:44:31pitrousetmessages: + msg166254
2012-07-16 07:08:15Thorneysetnosy: + jackdied
2012-04-14 04:15:16ezio.melottisetnosy: + ezio.melotti
2012-04-12 03:32:28Thorneysetfiles: + 12428.patch

messages: + msg158101
2012-02-20 12:50:24ncoghlansetmessages: + msg153779
2012-02-20 12:44:22ncoghlansetmessages: + msg153778
2011-12-09 15:38:38eric.araujosetnosy: + eric.araujo
messages: + msg149105
2011-12-09 09:53:35pitrousetversions: + Python 3.3, - Python 3.2
nosy: + pitrou

messages: + msg149082

stage: commit review
2011-07-30 06:08:14Thorneysetfiles: + functools2.diff

messages: + msg141425
2011-06-28 12:24:08rhettingersetmessages: + msg139359
2011-06-28 12:16:13ncoghlansetmessages: + msg139358
2011-06-28 10:07:17Thorneycreate