test_heapq C tests are not skipped when _heapq is missing

[ezio-melotti]

BPO	11910
Nosy	@brettcannon, @rhettinger, @jcea, @ncoghlan, @ezio-melotti
Files	issue11910.diff: Patch against 3.2. new_heapq_test.patch: Rough draft of simplified testing of multiple contexts. issue11910.diff: Patch that fixes import_fresh_module to return None and skips C test when _heapq is missing

^{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:

assignee = 'https://github.com/ezio-melotti'
closed_at = <Date 2011-05-09.05:48:58.086>
created_at = <Date 2011-04-23.02:04:18.944>
labels = ['type-bug', 'tests']
title = 'test_heapq C tests are not skipped when _heapq is missing'
updated_at = <Date 2011-05-09.05:48:58.085>
user = 'https://github.com/ezio-melotti'

bugs.python.org fields:

activity = <Date 2011-05-09.05:48:58.085>
actor = 'ezio.melotti'
assignee = 'ezio.melotti'
closed = True
closed_date = <Date 2011-05-09.05:48:58.086>
closer = 'ezio.melotti'
components = ['Tests']
creation = <Date 2011-04-23.02:04:18.944>
creator = 'ezio.melotti'
dependencies = []
files = ['21760', '21932', '21936']
hgrepos = []
issue_num = 11910
keywords = ['patch']
message_count = 14.0
messages = ['134293', '134368', '135483', '135488', '135497', '135503', '135504', '135507', '135516', '135537', '135538', '135560', '135562', '135563']
nosy_count = 6.0
nosy_names = ['brett.cannon', 'rhettinger', 'jcea', 'ncoghlan', 'ezio.melotti', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11910'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2', 'Python 3.3']

[ezio-melotti]

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.

[jcea]

Patch seems good. Ezio, can you commit?.

[ezio-melotti]

Attempting to fix import_fresh_module might be better.

[rhettinger]

Are you sure those tests are C specific? Please be careful about doing unnecessary complexification of this module's tests.

[ezio-melotti]

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.

[rhettinger]

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?

[rhettinger]

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.

[ezio-melotti]

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.

[rhettinger]

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.

[ezio-melotti]

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.

[ezio-melotti]

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

[python-dev]

New changeset c1a12a308c5b by Ezio Melotti in branch '2.7':
bpo-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

[python-dev]

New changeset 3ab1eb027856 by Ezio Melotti in branch '3.1':
bpo-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':
bpo-11910: merge with 3.1.
http://hg.python.org/cpython/rev/754bafe8db5f

New changeset 3e8f0111feed by Ezio Melotti in branch 'default':
bpo-11910: merge with 3.2.
http://hg.python.org/cpython/rev/3e8f0111feed

[python-dev]

New changeset 3cbbb2a7c56d by Ezio Melotti in branch '2.7':
bpo-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':
bpo-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':
bpo-11910: merge with 3.1.
http://hg.python.org/cpython/rev/4f3f67a595fb

New changeset 3449406fd04a by Ezio Melotti in branch 'default':
bpo-11910: merge with 3.2.
http://hg.python.org/cpython/rev/3449406fd04a