[Tests] Cleanup the tests of argparse. #11750

hobbestigrou · 2019-02-03T09:33:26Z

Remove useless import, remove useless assignment variables and be
pep8 compliant.

Remove useless import, remove useless assignment variables and be pep8 compliant.

SylvainDe

A few details.
As a general comment, one of the very important part of PEP 8 is at the very beginning:
"A Foolish Consistency is the Hobgoblin of Little Minds" (...) "However, know when to be inconsistent -- sometimes style guide recommendations just aren't applicable. " followed by "good reasons to ignore a particular guideline".

SylvainDe · 2019-02-03T17:08:56Z

Lib/test/test_argparse.py

@@ -471,8 +479,7 @@ class TestOptionalsAlternatePrefixCharsMultipleShortArgs(ParserTestCase):
                '-xyz',
                '+x',
                '-y',
-                '+xyz',
-    ]
+                '+xyz']


I think the original code is more idiomatic.

I agree with @SylvainDe

SylvainDe · 2019-02-03T17:09:36Z

Lib/test/test_argparse.py

-    argument_group_signatures   = TestHelpBiggerOptionals.argument_group_signatures
+    parser_signature = TestHelpBiggerOptionals.parser_signature
+    argument_signatures = TestHelpBiggerOptionals.argument_signatures
+    argument_group_signatures = TestHelpBiggerOptionals\


This looked more beautiful before.

I think that this change is good. IMO pep8 analyze should fail if we maintain the old style. Why never fail?

eamanu

I agree with @SylvainDe. Sometimes, fix pep8 problems will not be a good improve.
But core dev reviewers have the last word. :-)

Please review why the test are fail

eamanu · 2019-02-04T12:43:14Z

Lib/test/test_argparse.py

@@ -425,7 +431,9 @@ class TestOptionalsAlternatePrefixChars(ParserTestCase):
        Sig('::bar'),
        Sig('/baz', action='store_const', const=42),
    ]
-    failures = ['--bar', '-fbar', '-b B', 'B', '-f', '--bar B', '-baz', '-h', '--help', '+h', '::help', '/help']
+    failures = [


I think that is better use the first line to put values, and not start on new line. e.g.:

failures = [ '--bar', '-fbar', '-b B', 'B', '-f', '--bar B',
'-baz', '-h', '--help', '+h', '::help', '/help']

eamanu · 2019-02-04T12:43:26Z

Lib/test/test_argparse.py

@@ -471,8 +479,7 @@ class TestOptionalsAlternatePrefixCharsMultipleShortArgs(ParserTestCase):
                '-xyz',
                '+x',
                '-y',
-                '+xyz',
-    ]
+                '+xyz']


I agree with @SylvainDe

eamanu · 2019-02-04T12:43:48Z

Lib/test/test_argparse.py

-        self.assertRaises(ArgumentParserError, parser.parse_args,
-            ['-y', 'Y', '-z', 'Z'])
+        self.assertRaises(
+            ArgumentParserError, parser.parse_args, ['-y', 'Y', '-z', 'Z'])


Before is better.

eamanu · 2019-02-04T12:44:18Z

Lib/test/test_argparse.py

-    argument_group_signatures   = TestHelpBiggerOptionals.argument_group_signatures
+    parser_signature = TestHelpBiggerOptionals.parser_signature
+    argument_signatures = TestHelpBiggerOptionals.argument_signatures
+    argument_group_signatures = TestHelpBiggerOptionals\


I think that this change is good. IMO pep8 analyze should fail if we maintain the old style. Why never fail?

eamanu · 2019-02-04T12:47:27Z

Lib/test/test_argparse.py

 class TestHelpVersionAction(HelpTestCase):
    """Test the default help for the version action"""

    parser_signature = Sig(prog='PROG', description='description')
-    argument_signatures = [Sig('-V', '--version', action='version', version='3.6')]
+    argument_signatures = [Sig(
+        '-V', '--version', action='version', version='3.6')]


Same of the comments above.

csabella · 2019-03-04T13:37:28Z

Thanks for the work, but we don't generally make bulk changes like this. It generates churn in the codebase, and has the risk of inadvertently changing the meaning of the tests, to little actual benefit. Instead we modernize tests when we touch them for other reasons and are in a position to confirm that the changes do not change the meaning of the tests. (I realize that for most of your changes the meaning is trivially preserved...but when you make a lot of changes you are almost certain to make some mistakes...thus the resistance to doing bulk updates.)

As you can see, none of the continuous integration tests passed, which reinforces the fact that this change isn't as trivial as it may appear. For example, from the Travis check:

======================================================================
FAIL: test_subparser_title_help (test.test_argparse.TestAddSubparsers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_argparse.py", line 2070, in test_subparser_title_help
    self.assertEqual(parser.format_usage(),
AssertionError: 'usage: PROG [-h] [--foo] bar\n' != 'usage: PROG [-h] [--foo] bar {1,2} ...\n'
- usage: PROG [-h] [--foo] bar
+ usage: PROG [-h] [--foo] bar {1,2} ...
?                             ++++++++++
----------------------------------------------------------------------

the-knights-who-say-ni added the CLA signed label Feb 3, 2019

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Feb 3, 2019

[Tests] Cleanup the tests of argparse.

ce8677a

Remove useless import, remove useless assignment variables and be pep8 compliant.

hobbestigrou force-pushed the cleanup_test_argparse branch from 113bb22 to ce8677a Compare February 3, 2019 10:11

SylvainDe reviewed Feb 3, 2019

View reviewed changes

eamanu suggested changes Feb 4, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 4, 2019

csabella closed this Mar 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tests] Cleanup the tests of argparse. #11750

[Tests] Cleanup the tests of argparse. #11750

hobbestigrou commented Feb 3, 2019

SylvainDe left a comment

SylvainDe Feb 3, 2019

eamanu Feb 4, 2019

SylvainDe Feb 3, 2019

eamanu Feb 4, 2019

eamanu left a comment

eamanu Feb 4, 2019

eamanu Feb 4, 2019

eamanu Feb 4, 2019

eamanu Feb 4, 2019

eamanu Feb 4, 2019

csabella commented Mar 4, 2019

[Tests] Cleanup the tests of argparse. #11750

[Tests] Cleanup the tests of argparse. #11750

Conversation

hobbestigrou commented Feb 3, 2019

SylvainDe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eamanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Mar 4, 2019