GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-19382: Adding test cases for module tabnanny. by ultimatecoder · Pull Request #851 · python/cpython

Conversation

@ultimatecoder

@ultimatecoder ultimatecoder commented Mar 27, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

  • Description: Unit tests for standard module tabnanny. Tests are added for almost all the functionality except tabnanny.Whitespace class.

  • Reason leaving Whitespace:? I found the module contains mathematical calculations. I will try to understand it first and then write tests for it. I think it will be good to do that in another branch.

  • Testing strategy: Whitebox

  • BPOs

    • https://bugs.python.org/issue19382
    • https://bugs.python.org/issue19102

https://bugs.python.org/issue19382

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 27, 2017
@DimitrisJim

DimitrisJim commented Mar 27, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hi @ultimatecoder, you'll also need to remove tabnanny from the untested tuple in test_sundry. This is one of the failures AppVeyor (and soon Travis, I'd guess), reports.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added the tests Tests in the Lib/test dir label Mar 27, 2017
vadmium
vadmium reviewed Mar 27, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

Just write “captures exceptions” (or change arised → raised).

What is “tabsize”? Did you mean “tabnanny.check”?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

feeded → fed
dipicting → depicting

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

feeded → fed
Expacted → Expected

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

verifying → verify the
existance → existence

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

few → a few [otherwise, the implication is that most of the files have errors: few are error-free]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

giving

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

quiet mode?
“less” looks wrong, unless you meant “Should display less”

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

Don’t you get CRLF on Windows?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

In Python 3, b"%r" is equivalent to calling the “ascii” function. I expect the test will fail if the file path has non-ASCII characters. It may be better to only test that an error is being reported, but tolerate variations in the message.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vadmium vadmium Mar 27, 2017

Copy link
Copy Markdown
Member

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.

What if the path is non-ASCII? Again, just check the general form of the output and don’t be too specific.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder Mar 28, 2017

Copy link
Copy Markdown
Contributor Author

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 don't think this https://github.com/python/cpython/blob/master/Lib/tempfile.py#L144 generates anything Non-ASCII.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vadmium vadmium Mar 31, 2017

Copy link
Copy Markdown
Member

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.

No, but the OS can provide a parent directory for temporary files that has non-ASCII path components. It seems relatively common on Windows that the temporary directory is within a user’s profile, and the profile directory can be a localized non-ASCII name. Top Google result: https://bugzilla.mozilla.org/show_bug.cgi?id=260034.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

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.

Many thanks for affording time for reviewing this. I will agree with you on this point. I have updated code. Will you please review again?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vadmium

vadmium commented Mar 27, 2017

Copy link
Copy Markdown
Member

Check out the existing patches and reviews you linked. You may be repeating some of the same problems, e.g. CRLF on Windows.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch from d66ae89 to f9b9e5c Compare March 28, 2017 07:04
@ultimatecoder

ultimatecoder commented Mar 28, 2017

Copy link
Copy Markdown
Contributor Author

@vadmium Many thanks for your comments. I am not native English speaker. I have fixed the mistakes. I have added comments to some comments. Please review my fixes. Thanks!

@DimitrisJim Thanks for your comment. I have removed tabnanny from untested modules list. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch from f9b9e5c to d9953b9 Compare April 6, 2017 05:46
@vadmium

vadmium commented Apr 9, 2017

Copy link
Copy Markdown
Member

Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed. I’m not really set up to properly review and push stuff with the new Git Hub setup, so I will leave this to someone else, or for another time in the future.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

remove one empty line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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.

Removed this. This is Github glitch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Hum, I prefer to write mock.patch, please remove this import

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Please import tempfile and write tempfile.NamedTemporaryFile.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

IMHO this docstring is useless. Usually, we don't comment much tests, they are not intented to end users.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

If you only care of creating a temporary filename, use tempfile.mktemp() and then open() manually the file to create it. You can use open(tmpname, "x") to prevent race conditions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Instead of a dict, you can simply use a tuple (args, expected)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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.

Don't think we should kill the readability by converting it to tuple. How about collections.namedtuple?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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.

Or How about this way?

        tests = [
         #  (Function arguments , Expected output)
            (['first', 'second'], 'first second\n'),
            (['first']          , 'first\n'),
            ([1, 2, 3]          , '1 2 3\n'),
            ([]                 , '\n')
        ]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@merwok merwok Sep 1, 2017

Copy link
Copy Markdown
Member

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.

Keep tests simple:

for args, expected in [
    ([1], '1'),
  ]:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Feb 22, 2018

Copy link
Copy Markdown
Member

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, keep them simple. And don't use namedtuples except for keeping backwards-compatibility with an interface that used to take an indexed object and now takes an object with attributes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Not sure that this docstring is super useful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 don't think that it's worth it to declare one test case to test a single class/function. Try to put most or all tests into a single test case called "TabnannyTestCase".

TestCheck deserves its own test case since it has a setUp() method.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Please move this list into the method directly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Unit tests must have zero side effect: you must save the old verbose value and restore it. Example:

self.addCleanup(setattr, tabnanny, 'verbose', tabnanny.verbose)
tabnanny.verbose = 0

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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.

Aha! a geek way 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

No need to make this method private, remove "_" prefix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Again, I'm not sure that a long comment is needed for a simple unit test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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 it is explaining why the method is here. Suggest a way for improving the docstring.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Feb 22, 2018

Copy link
Copy Markdown
Member

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.

At least ditch the whole Arguments section; we don't use that style in the stdlib.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed May 4, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Who removes the temporary file? I suggest to put create_tmp_python_file() in the test case (or in a base class if it's needed by multiple test cases) and use self.addCleanup(support.unlink, tmpfile) to make sure that the temporary file is removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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.

Adding it to a method will be a poor idea. If I will add it to base class then we will have a base class with one method. Will you accept that?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Feb 22, 2018

Copy link
Copy Markdown
Member

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, if the method needs to access information that will be on the instance then a class with a single method is fine. But if you aren't reusing it then just inline the function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner requested changes May 4, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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 like the overall change, but I have a long list of requests :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

IMHO the docstring is useless.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Move this assert out of the with block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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 can't move this out.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Feb 22, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Why not? Is tokenize.generate_tokens() lazy (which isn't documented)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder Mar 18, 2018

Copy link
Copy Markdown
Contributor Author

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.

@brettcannon The tokenize.generate_tokens() is a lazy function (generator). Can you help to identify a way which allows putting assertRaises outside the with? Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

Use universal_newlines=True to normalize newlines. I also prefer to use text rather than bytes for stdout/stderr.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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 don't think these chain of methods, assert_python_ok -> _assert_python -> run_python_until_end is allowing to pass universal_newlines keyword argument!

Reference: https://github.com/python/cpython/blob/master/Lib/test/support/script_helper.py#L94

Are you suggesting to replace script_helper.assert_python_ok() with subprocess.Popen() here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

assert_python_ok() already tests that returncode==0.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

make this method public

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner May 4, 2017

Copy link
Copy Markdown
Member

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.

For better readability, you can use textwrap.dedent() and a multiline """...""" indented string. See test_faulthandler for examples.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder May 5, 2017

Copy link
Copy Markdown
Contributor Author

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 a nice suggestion 👍 Are you expecting these multiline strings to convert into a regular expression? I think this way just looks fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
JelleZijlstra
JelleZijlstra reviewed May 5, 2017
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@JelleZijlstra JelleZijlstra May 5, 2017

Copy link
Copy Markdown
Member

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.

witnesses, not whitenesses

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ultimatecoder reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@brettcannon brettcannon added the awaiting changes label Feb 21, 2018
@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder

ultimatecoder commented Feb 21, 2018

Copy link
Copy Markdown
Contributor Author

@brettcannon Many thanks for your comment. I had tried to comment back at confusing points. I think @vstinner lost his interest in my solution. What efforts should I do to make it mergeable? Have a great day!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

@ultimatecoder if you think you have addressed all of the comments left by @vstinner then just leave a comment that says I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder

ultimatecoder commented Feb 22, 2018

Copy link
Copy Markdown
Contributor Author

@brettcannon I have done the requested changes. There were few which I would like to discuss with @vstinner according to his availability. He has pointed out few changes on which I am confused about. I have tried to write back as a part of reply comment in response to improvement comment. I hope I made a clear statement about the situation. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder

ultimatecoder commented Mar 1, 2018

Copy link
Copy Markdown
Contributor Author

@brettcannon Many thanks for your comments. I am under little work pressure. Please expect answers/improvements at the end of this week. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder

ultimatecoder commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

@brettcannon Reminding you for this PR. All the best for sprints 👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon

brettcannon commented May 15, 2018

Copy link
Copy Markdown
Member

@ultimatecoder no need as this is the top of the list for when I actually get time to start reviewing PRs 😉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brettcannon
brettcannon previously requested changes May 15, 2018
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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.

Nothing fundamentally wrong, just some things to help make the tests easier to manage in future years.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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 would use subtests here: https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 14, 2018

Copy link
Copy Markdown
Member

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.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

Use f-strings everywhere. 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 14, 2018

Copy link
Copy Markdown
Member

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.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

To make this easier to read, I would create the context managers outside the with statement:

file1 = TemporaryPyFile(...)
file2 = TemporaryPyFile(...)
with file1 as file1_path, file2 as file2_path:
    ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 14, 2018

Copy link
Copy Markdown
Member

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.

For this specific case, IMHO "file1 = ..." is better.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

Move the TemporaryPyFile instantiation to its own line so you don't have to spread over multiple lines. (Same of the other lines where you were forced to spread a with statement over multiple lines.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 14, 2018

Copy link
Copy Markdown
Member

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 looked at the new code and it's fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

Pull this up to the next line up (it's okay for it to spill over 80 columns).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

Don't worry too much about the exact output being formatted as it is. For instance, if someone tweaked the grammar of the sentence the test should still pass. Do still check for semantically important info, though, like that the offending line is in the output.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 14, 2018

Copy link
Copy Markdown
Member

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.

Maybe the test is too strict, but IMHO we have been too pendantic on this nice PR which has been proposed 3 months ago. tabnanny has currently no test. If the test fails tomorrow, we will just fix the test. No need to write the perfect test here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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 don't want to tie yourself to exception messages as they do change between versions. Just care about the right exception being raised.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder Jun 10, 2018

Copy link
Copy Markdown
Contributor Author

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 agree, but the code does not raise an exception reference: https://github.com/python/cpython/blob/master/Lib/tabnanny.py#L99
What way will be better to assert the working of mentioned function?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@brettcannon brettcannon May 15, 2018

Copy link
Copy Markdown
Member

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.

Sub tests would be great here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jun 11, 2018

Copy link
Copy Markdown
Member

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.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented May 15, 2018

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels May 15, 2018
@ultimatecoder

ultimatecoder commented May 16, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@brettcannon I am a bit busy with present work. I am sure, I will not be able to complete during this sprints. But I will be able to improve according to your comments in few days. Thanks for reviewing :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon

brettcannon commented May 16, 2018

Copy link
Copy Markdown
Member

@ultimatecoder no rush! I'm not expecting to take quite so long to do future reviews as I only need to review relative future changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch from dcf42e5 to ebff66b Compare June 10, 2018 04:32
vstinner
vstinner reviewed Jun 10, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Please remove Lib/test/.test_tabnanny.py.swo from your PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed Jun 10, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Oh, I am sorry, I forgot about your old PR :-( Here is a new review. Your PR is very close to be ready to be merged. Sorry again about the slow review :-(

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner Jun 10, 2018

Copy link
Copy Markdown
Member

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.

Sorry but this style is not compliant with PEP 8, please strip spaces before ",".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner Jun 10, 2018

Copy link
Copy Markdown
Member

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 should always remove the created file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner Jun 10, 2018

Copy link
Copy Markdown
Member

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 may use:

proc = script_helper.spawn_python('-m', 'tabnanny', *args, text=True)
out, err = proc.communicate()
self.assertEqual(out, stdout)
self.assertEqual(err, stderr)
self.assertEqual(proc.returncode, 0)

text=True normalizes newlines to \n (Unix EOL).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch from db139c5 to 83083c9 Compare June 10, 2018 11:44
@ultimatecoder

ultimatecoder commented Jun 10, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner Thanks for re-observing this. I am working on the improvements suggested by you and @brettcannon 😃
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder

ultimatecoder commented Jun 10, 2018

Copy link
Copy Markdown
Contributor Author

@brettcannon I have updated the changes according to your comments. There is one comment in which you mentioned to assert the exception and not the output. I have added a reply to that comment with a reference. I request to provide your input on that. Thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch 2 times, most recently from a2be776 to e9f48d6 Compare June 10, 2018 12:22
@ultimatecoder

ultimatecoder commented Jun 10, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner I have improved the code according to your suggestion. I request to re-review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Jun 11, 2018
View reviewed changes
Comment thread Lib/test/test_tabnanny.py Outdated

@vstinner vstinner Jun 11, 2018

Copy link
Copy Markdown
Member

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'm not sure that returning True is correct here. I just suggest to remove the "return" line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ultimatecoder ultimatecoder Jun 14, 2018

Copy link
Copy Markdown
Contributor Author

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 agree, returning True isn't making sense here. Thanks for commenting.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ultimatecoder
[ADD] Adding test cases for module tabnanny. Testing stratergy:
86b9ad2 
Whitebox.
@ultimatecoder
ultimatecoder force-pushed the master-add-test-tabnanny-module branch from e9f48d6 to 86b9ad2 Compare June 14, 2018 04:25
@ultimatecoder

ultimatecoder commented Jun 14, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner @brettcannon I have updated the code according to your comments. I request to proceed for further review. Thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner added the skip news label Jun 14, 2018
@vstinner
vstinner dismissed brettcannon’s stale review June 14, 2018 07:04

I checked all latest Brett's comments: all fixed

vstinner
vstinner approved these changes Jun 14, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jun 14, 2018
@vstinner
vstinner merged commit dfa9643 into python:master Jun 14, 2018
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 14, 2018
@ultimatecoder

ultimatecoder commented Jun 14, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner Thanks for taking time and reviewing this PR.

@vstinner Thank you for reviewing your PR and inspiring for continuing.

I have learned many this at the time of creating, maintaining this Patch.
brettcannon reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@vadmium vadmium vadmium left review comments
@vstinner vstinner vstinner approved these changes
@brettcannon brettcannon brettcannon left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@ultimatecoder @DimitrisJim @vadmium @brettcannon @bedevere-bot @vstinner @merwok @JelleZijlstra @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.