classification
Title: test_heapq C tests are not skipped when _heapq is missing
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.1, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: brett.cannon, ezio.melotti, jcea, ncoghlan, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2011-04-23 02:04 by ezio.melotti, last changed 2011-05-09 05:48 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
issue11910.diff ezio.melotti, 2011-04-23 02:04 Patch against 3.2. review
new_heapq_test.patch rhettinger, 2011-05-08 02:14 Rough draft of simplified testing of multiple contexts.
issue11910.diff ezio.melotti, 2011-05-08 18:36 Patch that fixes import_fresh_module to return None and skips C test when _heapq is missing review
Messages (14)
msg134293 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-23 02:04
When _heapq is missing, test_heapq still runs both the Py and the C tests instead of skipping the C ones.  The attached patch skips the C tests when _heapq is missing.
msg134368 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2011-04-25 02:18
Patch seems good. Ezio, can you commit?.
msg135483 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-07 17:02
Attempting to fix import_fresh_module might be better.
msg135488 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-05-07 17:33
Are you sure those tests are C specific?  Please be careful about doing unnecessary complexification of this module's tests.
msg135497 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-07 18:05
If they are not C specific they should be moved to the base class (TestHeap).  TestHeapC and TestHeapPython should contain only tests specific to the C and Python versions respectively.

The goal here is to make sure that they are run once with the Python version, and again with the C version -- but only if the C version is available.
Currently, if _heapq is missing, the C tests (i.e. TestHeapC) are run anyway using the Python module, when instead they should be skipped.

c_heapq = import_fresh_module('heapq', fresh=['_heapq']) should be fixed to try to import _heapq and return None if the import fails, so that a @skipUnless(c_heapq, 'test requires the _heapq module') decorator can be added to TestHeapC.
See also http://www.python.org/dev/peps/pep-0399/

This can be fixed in 2.7 first.
msg135503 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-05-07 21:30
Some of the tests were incorrectly marked as being C specific.  I've fixed that on the 2.7 branch.  Re-assigning back to Ezio.

Overall, I don't think the current approach to testing both paths is elegant.  Is there some alternative approach to running suites in two different context without adding .module indirections and multiple subclasses throughout the code?
msg135504 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-05-07 21:39
I'm imagining a cleaner testing style, like this:

class TestHeap(unittest.TestCase):

    def test_nsmallest(self):
        self.assertEqual(heapq.nsmallest(3, range(10)), [0,1,2])
        ...

    @test_support.requires('_heapq')
    def test_comparison_operator(self):
        ...

def test_main(verbose=None):
    test_classes = [TestHeapPython, TestErrorHandling]
    test_support.run_unittest(*test_classes)

    test_support.reload('heapq', hiding='_heapq')
    test_support.run_unittest(*test_classes)

Ideally, we should be able to hide individual methods and be able to mark entire test classes with decorators for required features.
msg135507 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-07 21:48
Thanks (for the record the changeset is a8b82c283524).

> Overall, I don't think the current approach to testing both paths
> is elegant.
That's the most elegant way we have now.  If all the Python tests pass for the C version too, a base class could be avoided and the C tests could inherit directly from the Python ones (possibly adding additional C-specific tests).  I actually prefer this way because the Python tests should be an invariant of all the Python implementations and additional tests for the accelerations can be created from them with appropriate skip decorators, e.g.:
class PythonFooTests(TestCase):
    ...

@skipUnless(is_cypthon and c_foo)
class CFooTests(PythonTest):
   ...

@skipUnless(is_jython and j_foo)
class JFooTests(PythonTest):
   ...

This also avoid to list the tests explicitly at the end to exclude the base class (e.g. currently we have to exclude TestHeap, and list TestHeapC and TestHeapPy).


We could come up with some smart class decorator that runs a set of tests with and without accelerations, but it will make more difficult to add tests specific to the C or Python versions.
msg135516 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-05-08 02:14
Attaching a rough draft of a way to simplify dual path testing.  The idea is that the suite shouldn't have to be rewritten with self.module.heapify(...) references throughout.  Instead, tests are written normally and the only thing that changes in the context that they are executed in (not much different that if we simply commented out the import of c accelerator code).

Several things need work:
1) There needs to be a way to run only one block of tests if _heapqmodule isn't built.

2) the unittest.skipUnless decorator doesn't work (it does a static computation during compilation rather than a dynamic test during execution).  The attached rough draft code works around this by putting the skip logic inside the test.  This should be done more cleanly.

3) It would be great if there were a way to test unittest's test runner which version is being tested so that if there is a failure, it's obvious which context is being tested.

The patch is a proof-of-concept that would need polishing before becoming the preferred way of doing multiple path testing.
msg135537 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-08 18:36
Attached a patch that fixes import_fresh_module to return None when _heapq is missing and skips the C test when _heapq is missing.

I also added an additional test to verify that the functions in c_heapq are really C functions and the ones in py_heapq are really Python functions.
msg135538 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-08 18:46
BTW, if the fix for import_fresh_module is OK, it should be committed separately.
The tests I added to check that the C functions are really C functions could stay in test_heapq or they could be moved to test_test_support (see #11049), possibly together with tests for other modules like json/_json.  These tests are making sure that import_fresh_module works correctly but on the other hand they are also a precondition for a meaningful run of the following heapq tests.
msg135560 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-05-09 03:28
New changeset c1a12a308c5b by Ezio Melotti in branch '2.7':
#11910: change import_fresh_module to return None when one of the "fresh" modules can not be imported.
http://hg.python.org/cpython/rev/c1a12a308c5b
msg135562 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-05-09 03:44
New changeset 3ab1eb027856 by Ezio Melotti in branch '3.1':
#11910: change import_fresh_module to return None when one of the "fresh" modules can not be imported.
http://hg.python.org/cpython/rev/3ab1eb027856

New changeset 754bafe8db5f by Ezio Melotti in branch '3.2':
#11910: merge with 3.1.
http://hg.python.org/cpython/rev/754bafe8db5f

New changeset 3e8f0111feed by Ezio Melotti in branch 'default':
#11910: merge with 3.2.
http://hg.python.org/cpython/rev/3e8f0111feed
msg135563 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-05-09 04:30
New changeset 3cbbb2a7c56d by Ezio Melotti in branch '2.7':
#11910: Fix test_heapq to skip the C tests when _heapq is missing.
http://hg.python.org/cpython/rev/3cbbb2a7c56d

New changeset 677ee366c9f5 by Ezio Melotti in branch '3.1':
#11910: Fix test_heapq to skip the C tests when _heapq is missing.
http://hg.python.org/cpython/rev/677ee366c9f5

New changeset 4f3f67a595fb by Ezio Melotti in branch '3.2':
#11910: merge with 3.1.
http://hg.python.org/cpython/rev/4f3f67a595fb

New changeset 3449406fd04a by Ezio Melotti in branch 'default':
#11910: merge with 3.2.
http://hg.python.org/cpython/rev/3449406fd04a
History
Date User Action Args
2011-05-09 05:48:58ezio.melottisetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.1
2011-05-09 04:30:37python-devsetmessages: + msg135563
2011-05-09 03:44:48python-devsetmessages: + msg135562
2011-05-09 03:28:58python-devsetnosy: + python-dev
messages: + msg135560
2011-05-08 18:52:00ezio.melottisetnosy: + ncoghlan
2011-05-08 18:46:51ezio.melottisetmessages: + msg135538
2011-05-08 18:36:47ezio.melottisetfiles: + issue11910.diff

messages: + msg135537
2011-05-08 02:14:57rhettingersetfiles: + new_heapq_test.patch

messages: + msg135516
2011-05-07 21:48:08ezio.melottisetmessages: + msg135507
2011-05-07 21:39:45rhettingersetmessages: + msg135504
2011-05-07 21:30:40rhettingersetassignee: rhettinger -> ezio.melotti
messages: + msg135503
2011-05-07 18:05:43ezio.melottisetmessages: + msg135497
2011-05-07 17:33:35rhettingersetassignee: ezio.melotti -> rhettinger

messages: + msg135488
nosy: + rhettinger
2011-05-07 17:02:17ezio.melottisetassignee: ezio.melotti
messages: + msg135483
2011-04-25 02:18:33jceasetnosy: + jcea
messages: + msg134368
2011-04-23 02:05:13ezio.melottisetversions: + Python 2.7, Python 3.2, Python 3.3
nosy: + brett.cannon

components: + Tests
type: behavior
stage: patch review
2011-04-23 02:04:18ezio.melotticreate