gh-103558: Add coverage tests for argparse #103570
Conversation
There was a problem hiding this comment.
Thanks a lot, big work!
However, I think that we should not change version and parents validation just yet. Let's focus on tests itself for now, without any changes that can actually break stuff.
Because right now, we gain a bit of coverage and risk the backwards compatibility. I don't think that it is worth it.
| @@ -1789,13 +1800,11 @@ def identity(string): | |||
|
|
|||
| # add parent arguments and defaults | |||
| for parent in parents: | |||
| if not isinstance(parent, ArgumentParser): | |||
There was a problem hiding this comment.
Are there any other options on what can be actually passed here? Why it was like that? It is introduced in a pretty old commit without any mentions of "why": 698a18a
Maybe some structural subtyping? optparse?
However, it is documented that parents must be a list of ArgumentParser objects:
parents - A list of ArgumentParser objects whose arguments should also be included
So, I guess it is ok.
Lib/test/test_argparse.py
Outdated
| parser = ErrorRaisingArgumentParser(prog='PROG') | ||
| parser.add_argument('--foo', nargs="*") | ||
| parser.add_argument('foo') | ||
| with io.StringIO() as buf, redirect_stderr(buf): |
There was a problem hiding this comment.
We have with test.support.captured_stderr(): instead, it is easier to use and is more preferable.
Lib/argparse.py
Outdated
| @@ -1124,7 +1128,7 @@ class _VersionAction(Action): | |||
|
|
|||
| def __init__(self, | |||
| option_strings, | |||
| version=None, | |||
| version, | |||
There was a problem hiding this comment.
I think that this change can influence user-defined subclasses in a breaking way.
There was a problem hiding this comment.
You mean if user builds a class based on _VersionAction? The class is not documented and should be an implementation detail, but yes, it the users inherits the class this might break their code.
I guess that's the fundamental question for argparse - do we change it at all? Because the realitiy is, if we change it in any kind of way, we might break someone's code, even if it's a bug fix. We can improve the test coverage, but what's right and what's wrong? A wrong test is worse than no test, because the next person trying to fix it would be scared away.
Take #103498 as an example, assume we found the missing coverage there, what would we do? Do we write a "coverage test" that honors the current behavior even if it's obviously wrong according to the documentation?
We will find issues when we try to write comprehensive coverage tests, I think that's inevitable. Always keeping the current behavior would make writing tests more difficult (and to be honest, less meaningful).
However, I can revert the code change and just create a new issue for this(I still think this is a bug).
|
@sobolevn do we want a large PR with as many new test cases as possible while we are discovering new issues from |
|
@sobolevn and @hauntsaninja , this PR is not actively being worked on, but the tests still have worth. Should we consider merge them in for now? |
|
@hauntsaninja it looks like we need a core dev to review this. Could you give it a review? |
There was a problem hiding this comment.
This is great, thanks for working on this!
I made a couple changes, but all trivial (left some more details in individual commit titles):
a) removed some incorrect copy pasted docstrings
b) added back version action tests since we're not changing behaviour there anymore
c) changed some of the comments. I'm not very familiar with argparse and don't understand the code well enough to prove the _MutuallyExclusiveGroup._remove_action claim myself. Since it's not important to this PR I removed the comment for now
Current coverage at:
I also commented on some dead code - they are not reachable unless we explicitly call the private methods directly. I'm hesitated to do that.
From my point of view, the coverage of this file is pretty good, the missing lines are not even "corner". Most of them are simply impossible to reach unless the user explicitly trying to (meaning not using the public APIs in the documentation, but in that case, a lot of stuff can crash).
I have made two changes to the source code besides the comments:
versionaction, we explicitly mentioned in the docs that it expects aversionkeyword argument. So I made it fail atadd_argument()instead ofparse_args(). This will only change the failing point when the user does not specifyversionargument as the documentation says. I believe this actually makes more sense because we know it won't work if they do not specify it. We should not wait until the user send a--versioncommand to let them know. (the original code access parser.version which is not even a thing)parentargument toargparse.ArgumentParser(), it should expect a list ofArgumentParseraccording to the docs. If it's anArgumentParserobject, it will have_defaultsattribute, unless the user explicitly deletes it, which does not have any realistic use case. So instead of ignoring the attribute missing(which we can achieve with normal usage), I checked the object type instead.argparsemodule #103558