bpo-30617: IDLE: docstrings and unittest for outwin.py by csabella · Pull Request #2046 · python/cpython

csabella · 2017-06-09T21:58:12Z

https://bugs.python.org/issue30617

mention-bot · 2017-06-09T21:58:15Z

@csabella, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbkaiser, @rhettinger and @terryjreedy to be potential reviewers.

mlouielu · 2017-06-14T09:21:41Z

This is a duplicate requires('gui') since you have do it inside the setUpClass

mlouielu

IMO, if we are trying to add docstring or unittest, we should only add docstring and unittest, and not need to move other code, for example, the _file_line_helper and others.

Otherwise, it will be a refactor, and should be mention in the commit message (and title). If the refactoring is a must, I will prefer to split it into to two PR.

And the string quote should be consist. If we choice to use ', it will all be ' in the new code.

mlouielu · 2017-06-14T09:27:15Z

Why move out the internal parts? We can access these by OutputWindow.file_line_pats

I moved the _file_line_helper and these values to the module level per Terry's suggestion on the bug tracker.

@csabella dh, I saw it. I think it should be another issue to deal with these codes changed.

I agree that it would have been better in certain respects to start with a pure docstring patch, then tests of code as is, then a code fix-up and movement patch. This is more or less what we did with configdialog. This would be at least 2, maybe 3 issues. However, I have reviewed it as it is.

Nothng to do.

mlouielu · 2017-06-14T09:37:39Z

Maybe add another filename to test, and the docstring that explicit says this will only return False

csabella · 2017-06-14T15:42:55Z

@mlouielu I haven't made a separate PR yet, but I've made the other changes you suggested. I also added deletes for the mocks. One thing I wanted to ask is if you could help with the errors I'm getting when running the tests (warning: callback failed in WindowList <class '_tkinter.TclError'> : invalid command name ".!menu.windows"). I don't know enough to understand where to look to figure this out or what to change. Would you have any suggestions on what I can look at or how I can trace it? Thanks!

terryjreedy

The major issue is the test failure when the entire suite is run, which I will discuss separately.

terryjreedy · 2017-08-27T03:28:48Z

+        cls.text = w.text = Text(root)
+
+    @classmethod
+    def tearDownClass(cls):


As discussed on bpo-31284, msg 300912, adding cls.window.close() stops the error message on Windows.

Done.

terryjreedy · 2017-08-27T03:48:36Z

    # Customize EditorWindow
-
    def ispythonsource(self, filename):
+        "Control colorization of Python source."


"Python source is only part of output: do not colorize."
delete comment.

Done.

terryjreedy · 2017-08-27T03:49:17Z


    def maybesave(self):
+        "Customize EditorWindow to not display save file messagebox."
        # Override base class method -- don't ask any questions


Delete comment - replaced by docstring.

Done.

terryjreedy · 2017-08-27T03:53:30Z

-        ("Go to file/line", "<<goto-file-line>>", None),
-    ]
+        Provide file flush functionality for the window.
+        """


Condense to "No flushing needed as write() directly writes to widget."

Done.

terryjreedy · 2017-08-27T04:10:44Z

-        except TypeError:
-            return None
+        self.flist.gotofileline(filename, lineno)
+        return None


By PEP8, the returns were fine as they were. " Either all return statements in a function should return an expression, or none of them should." and none of them did.

Done.

terryjreedy · 2017-08-27T04:48:09Z

I agree that it would have been better in certain respects to start with a pure docstring patch, then tests of code as is, then a code fix-up and movement patch. This is more or less what we did with configdialog. This would be at least 2, maybe 3 issues. However, I have reviewed it as it is.

Nothng to do.

bedevere-bot · 2017-08-27T04:50:14Z

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 didn't expect the Spanish Inquisition!. 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.

And if you don't make the requested changes, you will be put in the comfy chair!

terryjreedy · 2017-08-27T05:35:52Z

+
+        w.flist = mock.Mock()
+        gfl = w.flist.gotofileline = Func()
+        showerror = outwin.messagebox.showerror = Mbox_func()


If you patch the real module, the original showerror function must be saved and restored. I would rather put
showerror = messagebox.showerror in Outwin (and change the later use to self.showerror), and then patch the instance for the test with `w.showerror = Mbox_func().

Done.

terryjreedy · 2017-08-27T06:04:39Z

The test suite fails on both Appveyor and my system with
ERROR: setUpClass (idlelib.idle_test.test_outwin.OutputWindowTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\idlelib\idle_test\test_outwin.py", line 20, in setUpClass
    w = cls.window = outwin.OutputWindow(None, None, None, root)
  File "C:\projects\cpython\lib\idlelib\outwin.py", line 78, in __init__
    EditorWindow.__init__(self, *args)
  File "C:\projects\cpython\lib\idlelib\editor.py", line 269, in __init__
    self.askyesno = tkMessageBox.askyesno
AttributeError: module 'tkinter.messagebox' has no attribute 'askyesno'
----------------------------------------------------------------------

The problem in not with the messagebox import in outwin.py, as it persists with the import disabled. (This causes a later error when the above does not occur.) The test passes with line 269 commented out. Line 271 self.showerror = tkMessageBox.showerror is no problem.

from tkinter import messagebox; print(dir(messagebox)) prints a list that includes askyesno. If I put print(id(tkMessageBox), dir(tkMessageBox)) immediately before EditorWindowdef init and again at the top of init, it is the same object. askyesno is initially present but then missing (but nothing else is).

I believe that the problem is that we twice mocked askyesno in test_configdialog by replacing it and then deleting it. It is the same error as I reported above for showerrer here. The fix should save and restore. It can be a short PR for bpo-30780.

Added: I opened new issue bpo-31287. I fixed issue by localizing askyesno to classes and then masking with instance attribute in tests. Did same for outwin and showerror.

terryjreedy · 2017-08-27T06:05:01Z

News blurb also needed

terryjreedy · 2017-08-27T20:56:56Z

The update includes the fix from bpo-31287 that fixes the GUI test failure (no messagebox.askyesno). I added a new file. Unfortunately, it causes Appveyor to not run. Closing and opening did not either. A regular patch will trigger Appveyor.

Unless you post a patch first, I am going to make the fairly simple changes I requested.

…H-2046) Move some data and functions from the class to module level. Patch by Cheryl Sabella. (cherry picked from commit 998f496)

#3223) Move some data and functions from the class to module level. Patch by Cheryl Sabella. (cherry picked from commit 998f496)

csabella · 2017-08-27T23:52:39Z

Sorry, I was trying to figure out the whole thing with why messagebox.askyesno was disappearing. I still don't quite understand it. I guess if a function or class from another module is mocked, then it should be saved/restored instead of deleted?

Thanks for making the other changes.

terryjreedy · 2017-08-28T00:22:08Z

The configdialog test replaced the original askyesno function in the messagebox module with a Mock and then deleted the mock. Some test imported editor before that, and editor imported the same module object. By the time outwin imported editor, the messagebox module referenced by editor was deficient. Test_idle is run in one process.

What puzzled me is that when outwin imported messagebox, it was not the same module object. (It had a different id().) I did not try to work out why. Python imports, with caching in sys.modules, is a tricky business sometimes.

csabella · 2017-08-28T20:48:10Z

Thanks for explaining. I played around some more (using mock.patch) and I think I have a better understanding of this.

There's a line in the mock.patch documentation, under the The patchers section, that says They [patchers] automatically handle the unpatching for you, even if exceptions are raised. . So I tried adding @mock.patch('idlelib.configdialog.tkMessageBox.askyesno') to the test functions in the old (broken) version of the code. Using the mock.patch decorator or context manager allowed it to work without having to del, save the original, or make it a function. I didn't really 'get it' before, but working through this helps me understand the why instead of just copying code.

Move some data and functions from the class to module level. Patch by Cheryl Sabella.

the-knights-who-say-ni added the CLA signed label Jun 9, 2017

Mariatta added the tests Tests in the Lib/test dir label Jun 13, 2017

mlouielu reviewed Jun 14, 2017

View reviewed changes

mlouielu suggested changes Jun 14, 2017

View reviewed changes

csabella force-pushed the outwin branch 2 times, most recently from 2f964a4 to 4adf076 Compare July 21, 2017 12:46

csabella added 3 commits August 25, 2017 17:23

bpo-30617: IDLE: docstrings and unittest for outwin.py

cda1a34

bpo-30617: IDLE: docstrings and unittest for outwin.py

997dce2

bpo-30617: IDLE: docstrings and unittest for outwin.py

7094e95

csabella force-pushed the outwin branch from 4adf076 to 7094e95 Compare August 25, 2017 21:24

Improve tests.

5ae9b9c

terryjreedy requested changes Aug 27, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Aug 27, 2017

terryjreedy requested changes Aug 27, 2017

View reviewed changes

terryjreedy added 2 commits August 27, 2017 16:43

Merge remote-tracking branch 'origin/master' into pr_2046

b80e1a9

News blurb

97277c5

terryjreedy closed this Aug 27, 2017

terryjreedy reopened this Aug 27, 2017

Make changes I requested.

ca95099

terryjreedy approved these changes Aug 27, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Aug 27, 2017

terryjreedy merged commit 998f496 into python:master Aug 27, 2017

terryjreedy added a commit that referenced this pull request Aug 27, 2017

[3.6] bpo-30617: IDLE: docstrings and unittest for outwin.py (GH-2046) (

5c89c2f

#3223) Move some data and functions from the class to module level. Patch by Cheryl Sabella. (cherry picked from commit 998f496)

csabella deleted the outwin branch August 27, 2017 23:52

Mariatta removed the awaiting merge label Aug 29, 2017

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-30617: IDLE: docstrings and unittest for outwin.py (python#2046)

030f76b

Move some data and functions from the class to module level. Patch by Cheryl Sabella.

Uh oh!

Conversation

csabella commented Jun 9, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jun 9, 2017

Uh oh!

mlouielu Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

mlouielu left a comment

Choose a reason for hiding this comment

Uh oh!

mlouielu Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

csabella Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

mlouielu Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlouielu Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

csabella Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

csabella commented Jun 14, 2017

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 27, 2017

Uh oh!

terryjreedy Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy commented Aug 27, 2017

Uh oh!

terryjreedy commented Aug 27, 2017

Uh oh!

csabella commented Aug 27, 2017

Uh oh!

csabella commented Jun 9, 2017 •

edited by bedevere-bot

Loading

mlouielu Jun 14, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy Aug 27, 2017 •

edited

Loading

terryjreedy commented Aug 27, 2017 •

edited

Loading