Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-103558: Add coverage tests for argparse#103570
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-103558: Add coverage tests for argparse#103570
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sobolevn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot, big work!
However, I think that we should not changeversion andparents 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.
| # add parent arguments and defaults | ||
| forparentinparents: | ||
| ifnotisinstance(parent,ArgumentParser): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thatparents must be a list ofArgumentParser objects:
parents - A list ofArgumentParser 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') | ||
| withio.StringIO()asbuf,redirect_stderr(buf): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We havewith test.support.captured_stderr(): instead, it is easier to use and is more preferable.
Lib/argparse.py Outdated
| def__init__(self, | ||
| option_strings, | ||
| version=None, | ||
| version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that this change can influence user-defined subclasses in a breaking way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 forargparse - 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).
gaogaotiantian commentedApr 24, 2023
@sobolevn do we want a large PR with as many new test cases as possible while we are discovering new issues from |
gaogaotiantian commentedMay 30, 2023
@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? |
sobolevn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I think so!
furkanonder left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
gaogaotiantian commentedJun 5, 2023
@hauntsaninja it looks like we need a core dev to review this. Could you give it a review? |
hauntsaninja left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
Uh oh!
There was an error while loading.Please reload this page.
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 thedocs 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 thedocs. 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