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).

It is also in the spirit of dogfooding.

Do you want to propose a patch?
regrtest has no tests, so switching to argparse might cause more harm than good right now.

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.

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

Thanks, Senthil.  That is my plan.  I should be able to have code with tests in no later than a week.

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.

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.

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.

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.

Please leave this for Python 3.4 -- it is not a bugfix.

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?

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.

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.

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.

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.

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>
> _______________________________________

> 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.

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?

> 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.

By the way, I am in the process of cleaning up the patch a bit.

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.

Here is an updated, improved patch.

Anton, you are free to make that issue and propose a patch :)

New patch is LGTM.

Chris, would you commit it?

> 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.)

Agree with both points

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.

(I could, of course, be wrong :)

Yes, please don't make something called regrtestlib.py. Some of us like the bash completetion to work with on a small prefix. :)

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.

I think bitdancer's suggestion of leaving it in regrtest.py is a good one.

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.

Here is an alternative patch with a cleaner diff (keeping the help-related strings at the top before the import statements).

I posted some review comments.

Thanks.  I replied.

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.)

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).

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

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.

Good step!

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