bpo-29984: Improve 'heapq' test coverage #992
Conversation
|
@rkday, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rhettinger, @tiran and @ezio-melotti to be potential reviewers. |
| @@ -135,6 +153,13 @@ def test_heappushpop(self): | |||
| x = self.module.heappushpop(h, 11) | |||
| self.assertEqual((h, x), ([11], 10)) | |||
|
|
|||
| def test_heappop_max(self): | |||
There was a problem hiding this comment.
What is the line being tested? heappop_max() just calls heappop_internal() which is also used by the regular heappop(). I prefer testing the latter because it is part of the public API. Likewise, the test should apply to both the C code and the pure python code (eventhough only the latter has a special case, the test should pass for both). Alternatively, if you specifically want to exercise heappop_max(), it would be best to do it through the public function that uses it, merge().
There was a problem hiding this comment.
It's
Line 187 in 43ba886
I did wonder if I could test this via merge, but it looks like merge has an optimisation where it never calls heappop_max on a one-item list (
Line 354 in 43ba886
I'll change this test to be called on self.module, but I can't see a way to hit that line of Python code by just going through the public API (though I could refactor it to only have a single return statement?).
There was a problem hiding this comment.
Actually, this already uses self.module, so I think this is does apply to/pass for both the C and Python code.
Lib/heapq.py
Outdated
| @@ -599,9 +599,3 @@ def nlargest(n, iterable, key=None): | |||
| from _heapq import _heappop_max | |||
| except ImportError: | |||
| pass | |||
|
|
|||
|
|
|||
| if __name__ == "__main__": | |||
There was a problem hiding this comment.
Please leave this in. I use it when doing maintenance for the module (we have many such cases in the standard library.
There was a problem hiding this comment.
Yep, will do.
- Is the rest of the change (to run the doctests as part of the test suite) useful or should I just revert the commit?
- Is there anything I should do to mark these lines as uncovered so that coveragepy reports 100%? (I looked, but the other modules with this snippet like difflib and pickle also don't have 100% coverage yet.)
There was a problem hiding this comment.
I saw that pragma: nocover gets used elsewhere (e.g.
Line 495 in 5affd23
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @rhettinger: please review the changes made to this pull request. |
This brings coverage of the heapq module to 100%.
The doctest change is a bit weird, so I've made that a separate commit so it can be straightforwardly backed out if it's too weird. I dug into why
tests.addTests(doctest.DocTestSuite(py_heapq))doesn't find any tests and it's because the comparison here doesn't return True, which I suspect is because the module is loaded withimport_fresh_module.(This is a pretty trivial change - I'm just improving test coverage somewhere to get started with contributing to Python, per https://docs.python.org/devguide/coverage.html)
https://bugs.python.org/issue29984