classification
Title: Use argparse instead of getopt in test.regrtest
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: a.kasyanov, asvetlov, benjamin.peterson, chris.jerdonek, ezio.melotti, georg.brandl, orsenthil, python-dev, r.david.murray, tshepang
Priority: normal Keywords: easy, patch

Created on 2012-07-09 05:26 by chris.jerdonek, last changed 2013-01-08 01:10 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue-15302-1.patch chris.jerdonek, 2012-07-11 06:30 review
issue-15302-2.patch chris.jerdonek, 2012-07-11 10:52 review
issue-15302-3.patch chris.jerdonek, 2012-12-24 18:21 review
issue-15302-4.patch chris.jerdonek, 2012-12-27 02:59 review
issue-15302-5.patch chris.jerdonek, 2012-12-27 03:16 review
issue-15302-6.patch chris.jerdonek, 2012-12-27 10:03 review
Messages (43)
msg165064 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-09 05:26
I think it would be an improvement to switch from using getopt to argparse in test.regrtest.  The code would be easier to maintain, it would give us more powerful options going forward, and it would improve the usability of the test command (e.g. nicer command-line help).
msg165065 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-09 05:28
It is also in the spirit of dogfooding.
msg165142 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-07-10 02:10
Do you want to propose a patch?
regrtest has no tests, so switching to argparse might cause more harm than good right now.
msg165152 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-10 03:50
Sure, if someone is open to reviewing it.

The parsing code doesn't seem to be doing anything too fancy right now.  I can decouple the parsing code and begin adding tests around parts that may need it more.  Increasing coverage will be easier going forward.
msg165167 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2012-07-10 06:37
On Mon, Jul 9, 2012 at 8:50 PM, Chris Jerdonek <report@bugs.python.org> wrote:>
> Sure, if someone is open to reviewing it.

Yes, please go ahead. I can review it and I believe others should be
able to as well. As a first short,  I think, it may be a good idea to
just replacing the short opt and long opt handling behavior and not
dwell too much into argparse features (just to ascertain that we are
not going through a new code-path. this I believe, may help assuage
Ezio's concern a bit  that regrtest.py lacks test and changes to them
should be carefully done at this point in time in release).  Or the
other idea is you may submit the patch and if it is significant, it
may go in early in 3.4
msg165169 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-10 06:45
Thanks, Senthil.  That is my plan.  I should be able to have code with tests in no later than a week.
msg165173 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-10 07:36
By the way, issue 15300 has a related patch that is ready to review today.

Assuming that one is okay, it would make sense to commit first because it overlaps with the changes I'll be doing here.

Issue 15305 is another related issue (also overlapping) that is probably worth discussing.
msg165200 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-07-10 20:04
As mentioned, the first step is to create some tests that can validate the current behavior, so that changes don't break things.  This is a non-trivial task.  I know from experience with a similar refactoring that even seemingly simple changes can have unexpected consequences, and that getting good functional test coverage (not code-line test coverage) is hard.  This is complicated by the fact that regrtest is an over-evolved mess.  My ideal is to move appropriate pieces of the functionality into unittest and make regrtest a wrapper around that, but obviously I haven't spent much time actually doing that.

I don't think that regrtest tests need to be run as part of the standard python test run, by the way, though I suppose they could be.
msg165228 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-11 06:30
Here is a patch that creates some unit tests for the existing getopt argument parsing code.

In response to the comments, I'm thinking of a less invasive approach that involves wrapping argparse's parse_args() to return getopt-like output (and in particular, to match test expectations).

In this way, we will be able to keep main() mostly unaffected while still being able to use argparse.
msg165247 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-11 10:52
Attached is a first version of a complete patch.

Note that I found three bugs in the current argument parsing code in the course of working on this patch: issue 15324, issue 15325, and issue 15326 (because of various typos in the getopt configuration).  All of these should be fixed by the current patch.
msg165287 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-07-12 07:46
Please leave this for Python 3.4 -- it is not a bugfix.
msg177978 - (view) Author: Anton Kasyanov (a.kasyanov) * Date: 2012-12-23 12:30
I've looked through the second patch and I'm not sure about how argparse usage was implemented here - parse_args() result is being converted to getopt-style list of (option, value) pairs.
 
Is there any sense in using argparse this way?
msg178006 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-23 18:54
The reason in part is because of the lack of unit tests of regrtest (as commenters above have noted).  By preserving the getopt interface, we can keep almost all of the untested code as is.

You should view the patch as a first step in refactoring to use argparse.  We can remove the conversion code and the main for loop in later steps.

Note to committers: I was meaning to rename regrlib.py when I was working on this.  Until regrtest-related code is in its own subpackage, I think the file name should begin with "regrtest" -- perhaps regrtester.py or regrtestlib.py.
msg178011 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-23 19:25
Hi Chris.
Today we had python sprint and I've guessed to Anton to refactor the patch in good way with properly setting default values from regrtest.main to argparse args. Then use proper argparse actions for manipulating that args.

After all we can use Namespace object returned from argparse.parse() or argparse.parse_known_args() if needed as input for next processing.

It will be big enough patch but I like to move it forward after double checking.

regrtestlib.py name is good to me.
msg178015 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-23 20:11
Yes, I agree with all of that but thought it would be easier to review if done incrementally as separate steps.  In any case, I will look for Anton's patch on the review tool in case I have any comments.
msg178024 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-12-24 00:33
I am -1 on doing this as one big refactoring, unless tests are written for regrtest first.  Incremental (over a period of weeks or months, so that the changes get some soak time each time) is I think acceptable even without tests, given that this is a test runner and not part of Python proper.
msg178052 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-24 12:02
Well. Do you like to apply Chris patch and wait for next step appear?

On Mon, Dec 24, 2012 at 2:33 AM, R. David Murray <report@bugs.python.org> wrote:
>
> R. David Murray added the comment:
>
> I am -1 on doing this as one big refactoring, unless tests are written for regrtest first.  Incremental (over a period of weeks or months, so that the changes get some soak time each time) is I think acceptable even without tests, given that this is a test runner and not part of Python proper.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue15302>
> _______________________________________
msg178062 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-24 14:55
> Do you like to apply Chris patch and wait for next step appear?

Just to clarify, I think this should read "apply Chris patch after updating/reviewing."  A couple file renames are needed, and I noticed a typo in a docstring.  Other changes may be needed since 5 months have elapsed.
msg178064 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-12-24 15:50
Yes, I apologize for not getting around to a review previously.

Chris: why regrlib.py at all?  Why isn't the code in regrtest.py?  And I'm not clear on why there is a desire to have regrtest be a package.  Did I miss that discussion?
msg178070 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-24 16:33
> why regrlib.py at all?  Why isn't the code in regrtest.py?

It was for a few related reasons.  It was primarily to make it easier to reason about testing regrtest, and to avoid at the outset any pitfalls that might arise from the circularity of regrtest testing itself.  For example, the regrtest module seems to carry a fair bit of state and top-level code, whereas the "library" module being added would be stateless.  It would also let us maintain an obvious line between what is tested and what is not.

There was no discussion of creating a package.  However, if we go this route I would foresee moving functionality from regrtest to the library module as we expand the amount of regrtest-support code under test.
msg178073 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-24 16:36
By the way, I am in the process of cleaning up the patch a bit.
msg178080 - (view) Author: Anton Kasyanov (a.kasyanov) * Date: 2012-12-24 17:31
So I see that there is no need in full refactoring now.

BTW, maybe someone should create an issue for full tests coverage of regrtest? Then it would be much easier to make a full refactoring.
msg178087 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-24 18:21
Here is an updated, improved patch.
msg178239 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-26 19:39
Anton, you are free to make that issue and propose a patch :)
msg178240 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-26 19:46
New patch is LGTM.
msg178241 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-26 19:46
Chris, would you commit it?
msg178243 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-26 19:54
> BTW, maybe someone should create an issue for full tests coverage of regrtest?

I think such an issue would need to be a meta-issue because it is a very large task (involving many patches) and may never be truly finished.

> Chris, would you commit it?

Sure, I should be able to get to it within a day or two.  (By the way, after this is committed, the use of a Namespace return value can be phased in gradually as a replacement for some options in the getopt for loop.  It need not be all or nothing.)
msg178244 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-26 19:55
Agree with both points
msg178245 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-12-26 19:58
I'm still -0 on making a new file.  regrtest will be running as __main__, so I don't see any reason the test file can't import it to test functions within it.
msg178246 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-12-26 19:59
(I could, of course, be wrong :)
msg178247 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-12-26 20:06
Yes, please don't make something called regrtestlib.py. Some of us like the bash completetion to work with on a small prefix. :)
msg178250 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-26 20:15
Benjamin, do you not want a new file at all, or are you just asking for a different name?  regrlib was the previous name unless you have another suggestion.
msg178251 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-12-26 20:18
I think bitdancer's suggestion of leaving it in regrtest.py is a good one.
msg178264 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-27 02:59
I'm attaching an updated patch incorporating David and Benjamin's suggestion.  (Thanks a lot for the feedback, by the way.)  I also added a test for the help option and refactored to avoid having to use context managers to check sys.argv, sys.stderr, etc.
msg178267 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-27 03:16
Here is an alternative patch with a cleaner diff (keeping the help-related strings at the top before the import statements).
msg178269 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-12-27 03:44
I posted some review comments.
msg178270 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-27 04:59
Thanks.  I replied.
msg178289 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-27 10:03
Updating patch after Benjamin's review.

In this new patch, in test_regrtest I now use the current, actual getopt code to test and demonstrate backwards compatibility.  Note that when I pasted the code, I also fixed the three typos in the current getopt code that are documented in the following three issues: issue15324, issue15325, issue15326.  These typos are checked by the following test cases: test_fromfile, test_match, and test_randomize.  (I will fix these three issues in versions prior to 3.4 independent of this issue.)
msg178315 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-27 20:09
Rietveld is erroring out on me again whenever I try to reply to a comment, so I'm posting my comment here.

On 2012/12/27 18:29:22, Benjamin Peterson wrote:
> > On 2012/12/27 04:44:33, Benjamin Peterson wrote:
> > > if val:
> > 
> > Again, we need this to match getopt behavior. Using "if val" would replace
> the
> > value provided for all options accepting a value with the empty string. In
> > particular, the test_fromfile() test case (and two others) would fail with
> that
> > change. On the flip side, getopt causes provided boolean options to show up
> as
> > having an empty string value in the return value which is why I'm setting val
> to
> > ''.
> 
> I suppose this stuff is temporary anyway until regrtest can actualy use a nice
> namespace.

Right. That part is a temporary bridge.

I updated the patch based on your original comments, by the way. The patch is a lot simpler now, including dropping the need to subclass ArgumentParser, taking out the old usage() function, simplifying the tests, and making the need for the tests more explicit (which also can be removed after switching to using a namespace object).
msg178355 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-28 02:55
New changeset 6e2e5adc0400 by Chris Jerdonek in branch 'default':
Issue #15302: Switch regrtest from using getopt to using argparse.
http://hg.python.org/cpython/rev/6e2e5adc0400
msg178357 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-28 03:13
Thanks again for your reviews, Benjamin (and others).

I created issue 16799 for the next phase of this process: changing regrtest.main() from operating on getopt-style parsed options to an argparse Namespace object.
msg178412 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-12-28 18:42
Good step!
msg179302 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-08 01:10
New changeset ce99559efa46 by Chris Jerdonek in branch 'default':
Issue #16854: Fix regrtest.usage() regression introduced in 6e2e5adc0400.
http://hg.python.org/cpython/rev/ce99559efa46
History
Date User Action Args
2013-01-08 01:10:39python-devsetmessages: + msg179302
2012-12-28 18:42:15asvetlovsetmessages: + msg178412
2012-12-28 03:13:28chris.jerdoneksetstatus: open -> closed
resolution: fixed
messages: + msg178357

stage: patch review -> resolved
2012-12-28 02:55:22python-devsetnosy: + python-dev
messages: + msg178355
2012-12-27 21:49:19chris.jerdonekunlinkissue15326 superseder
2012-12-27 21:47:53chris.jerdonekunlinkissue15325 superseder
2012-12-27 20:09:13chris.jerdoneksetmessages: + msg178315
2012-12-27 11:43:35chris.jerdonekunlinkissue15324 superseder
2012-12-27 10:03:57chris.jerdoneksetfiles: + issue-15302-6.patch

messages: + msg178289
2012-12-27 04:59:51chris.jerdoneksetmessages: + msg178270
2012-12-27 03:44:51benjamin.petersonsetmessages: + msg178269
2012-12-27 03:16:16chris.jerdoneksetfiles: + issue-15302-5.patch

messages: + msg178267
2012-12-27 02:59:06chris.jerdoneksetfiles: + issue-15302-4.patch

messages: + msg178264
2012-12-26 20:18:07benjamin.petersonsetmessages: + msg178251
2012-12-26 20:15:10chris.jerdoneksetmessages: + msg178250
2012-12-26 20:06:22benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg178247
2012-12-26 19:59:30r.david.murraysetmessages: + msg178246
2012-12-26 19:58:58r.david.murraysetmessages: + msg178245
2012-12-26 19:55:57asvetlovsetmessages: + msg178244
2012-12-26 19:54:19chris.jerdoneksetmessages: + msg178243
2012-12-26 19:46:51asvetlovsetmessages: + msg178241
2012-12-26 19:46:25asvetlovsetmessages: + msg178240
2012-12-26 19:39:34asvetlovsetmessages: + msg178239
2012-12-24 18:21:54chris.jerdoneksetfiles: + issue-15302-3.patch

messages: + msg178087
2012-12-24 17:31:07a.kasyanovsetmessages: + msg178080
2012-12-24 16:36:48chris.jerdoneksetmessages: + msg178073
2012-12-24 16:33:42chris.jerdoneksetmessages: + msg178070
2012-12-24 15:50:20r.david.murraysetmessages: + msg178064
2012-12-24 14:55:29chris.jerdoneksetmessages: + msg178062
2012-12-24 12:02:15asvetlovsetmessages: + msg178052
2012-12-24 00:33:35r.david.murraysetmessages: + msg178024
2012-12-23 20:11:18chris.jerdoneksetmessages: + msg178015
2012-12-23 19:25:25asvetlovsetmessages: + msg178011
2012-12-23 18:54:57chris.jerdoneksetmessages: + msg178006
2012-12-23 12:30:23a.kasyanovsetnosy: + asvetlov, a.kasyanov
messages: + msg177978
2012-07-13 19:29:21ezio.melottilinkissue15326 superseder
2012-07-13 19:29:02ezio.melottilinkissue15325 superseder
2012-07-13 19:28:01ezio.melottilinkissue15324 superseder
2012-07-13 19:00:09tshepangsetnosy: + tshepang
2012-07-12 07:46:52georg.brandlsetnosy: + georg.brandl

messages: + msg165287
versions: + Python 3.4, - Python 3.3
2012-07-11 13:05:55brett.cannonsetstage: needs patch -> patch review
2012-07-11 10:52:57chris.jerdoneksetfiles: + issue-15302-2.patch

messages: + msg165247
2012-07-11 06:30:55chris.jerdoneksetfiles: + issue-15302-1.patch
keywords: + patch
messages: + msg165228
2012-07-10 20:04:02r.david.murraysetnosy: + r.david.murray
messages: + msg165200
2012-07-10 07:36:13chris.jerdoneksetmessages: + msg165173
2012-07-10 06:45:30chris.jerdoneksetmessages: + msg165169
2012-07-10 06:37:10orsenthilsetnosy: + orsenthil
messages: + msg165167
2012-07-10 03:50:14chris.jerdoneksetmessages: + msg165152
2012-07-10 02:10:04ezio.melottisetnosy: + ezio.melotti
messages: + msg165142

type: enhancement
stage: needs patch
2012-07-09 05:28:09chris.jerdoneksetmessages: + msg165065
2012-07-09 05:26:22chris.jerdonekcreate