New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increased test coverage of test_urlparse #56790
Comments
Some very trivial tests to increase the test coverage on test_urlparse a bit; also changed a single line to use an ABC instead of attempting to use len() to verify that an object is "sequence-like" (as the comment put it). Mostly I’m trying to get my feet wet in submitting a patch in the manner suggested by the guide. (Curiously, the full test suite coverage report cites 99%, even though the path in question does run: coverage.py fails somehow to mark an else branch consisting of a lone continue statement.) Looking at this, I have some questions, but figured I might as well ask them along with a patch: First, whether this is appropriate use of ABCs in the library; second, whether it’s appropriate to submit related stuff like modified tests and (lightly) modified code in one patch, or whether I should rather open two issues. Third, and more generally, I’m wondering whether the tests are appropriate. These are trivial in scope and nature, but I would be interested to know for future reference (and perhaps it would be useful to mention in the docs?) what the policy is on this. Essentially, I encoded expectations of current behaviour as “correct” and covered the 30-odd statements in urllib/parse.py that were not already covered by a full test run (explicit coverage by test_urlparse is still much lower!) on the assumption that, as the coverage guide suggests, more coverage is always better – and even without domain expertise assurance that current behaviour is correct, it at least provides some assurance that changes in behaviour will be discovered. Still, it’s perhaps *too* easy to get hung up on those coverage percentages: Is it *always* better to cover more (keeping in mind the limitations of once-over coverage), or would contributors be better advised not to cover code unless they are very, very confident that current behaviour is correct? |
I haven't reviewed your tests, but a couple quick comments: we generally prefer duck typing to the use of isintance or ABCs, but sometimes the latter is better (it's a judgement call). I haven't done a deep dive in the code you modified, but from the looks of the code quoted in your patch I'd say that doing 'iter(v)' inside the try/except would be the way to find out if one can loop over the object, which appears to be the only aspect of sequenceness the code cares about. As for coverage, you are right that it is quite possible to get caught up in the statistics. That said, if you *don't* have domain knowledge, giving us a set of tests to look at and evaluate is better than not having such a set of tests. Pointing out any tests that you aren't sure about the validity of is helpful in any case. The overall goal of the test suite is to test the *API* of the library functions. This is so that alternate implementations can use it as a validation test suite (Sometimes we have CPython specific tests, in which case we mark them as such). So testing internal implementation details is not as helpful as testing behavior. If you find you have to use a "white box" test (one that pokes at the internals as opposed to making an appropriate call to the API), then the code you can't otherwise test becomes suspect and an appropriate subject for another issue ("what is this code for? I can't get it to trigger.") Finally, your point about comprehensive tests at least showing up behavior changes is valid. If you write tests that you aren't sure are "correct behavior", put in an XXX comment to that effect. If you just have no idea, you can mark a whole block of tests as "this improves coverage, I have no idea if the behavior is valid or not", and we'll either sort it out when we review or commit the tests or just leave the comment in. Thanks for working on this. |
It’s my pleasure — it’s very trivial, but hopefully it’ll get my feet wet and get me in a place where I am familiar enough with procedures and things to contribute something relevant. :) Attaching a modified patch with (1) reversion to duck typing in parse.py, and (2) a few comments added to the tests. At this point, it may even be worth reviewing when someone has the time. (While the tests are pretty trivial, and admittedly coverage driven, they do hit a few edge cases with blank values &c. that were previously not run.) |
Hi Petter, writing tests are ofcourse a good way to start. As long as the tests increase the coverage, those are most welcome. Thanks! |
Added suggested changes from review, removed (rather useless) repr test; left parse.py changes alone (see review comments for rationale) |
Peter’s patch now uses iter(thing) instead of len(thing) to assess sequenciness. I made this comment:
Peter’s reply:
I think iter may exhaust a non-repeatable iterable, whereas len would give a TypeError and not consume the iterable. iter(thing) being successful does not guarantee that a second iteration will succeed. I’ve had a look at the docstring and the reST docs, and they clearly say that sequences are supported, not arbitrary iterables.
Note that ABCs are not strict, “true” type checks like there is in other languages :)
Log viewing tools usually let you follow the history across renames, or otherwise let you see the history for a file that’s not anymore in the tip revision. Here it was Lib/urlparse.py. (Just saying this for your information, it does not matter anymore to check the history to find if the code pre-dates iterators.) |
On Sat, Jul 23, 2011 at 09:37:27AM +0000, Éric Araujo wrote:
Yeah. At the first cut, when I saw the suggestion of iter(), I thought As Eric points out, changing it to iter may cause some side-effects |
New changeset e171db785c37 by Senthil Kumaran in branch '3.2': New changeset fcccda3c546f by Senthil Kumaran in branch 'default': New changeset ed79da800b4a by Senthil Kumaran in branch '2.7': |
Thanks a lot for the patch, Petter Haggholm. I was initially hesitant to have separate tests for functions/methods But once I tried coverage report on the module and saw that |
Exhaustion of the iterator is easily solved by simply retaining a reference to it and iterating that (which is what I had in mind). However, I had not thought about the problem of an *in*exhaustable iterator, and to cover that case len is indeed better. |
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: