gh-75710: IDLE: Add tests to some editor.py functions by csabella · Pull Request #3669 · python/cpython · GitHub

Add docstrings and test for editor.py functions related to the reload from configdialog, creating the menubar, and applying keybindings.

https://bugs.python.org/issue31529

Issue: IDLE: Add docstrings and tests for editor.py reload functions #75710

I was trying to understand the editor functions accessed from configdialog so that I could test those and thought the best way to understand what was really happening was to document and test the functions being called.

Some notes:

I haven't done ResetColorizer because that seemed like its own rabbithole.
I had to add a windows-only test because os.startfile is a windows only function and wouldn't work on my system. Hope it works.
I was getting the timer_event CodeContext errors when running the tests, so I added a line to _close() in editor.py to unbind that event before deleting the text instance and it made the error go away. If it's the right fix, you may want to pull that out into its own PR.

Sorry that this is still massive. I really tried to limit it to what I needed to look at.

I will add a coverage line so we know how far we yet have to go.

I later did, which is part of the merge conflict.

Done, part of the merge conflict.

Amazing work adding so many tests for the editor window! I think these are sorely needed, and it's too bad they've been languishing here for a while now.

I've got a bunch of review comments, but overall this is a huge improvement, and I'd like to help get it through!

Add another blank line before the "Note:".

Or leave out 'Note:'. I am going to revise these added docstrings.

IMO in this case, the rest of the doc-string after the first line should be a comment instead, since it describes the method's code rather than it's purpose.

The problem goes deeper. There should be separate window-with-menu, frame, and text classes, and possibly an IDLE-menu class with a docstring that explains the menu system. And it maybe should be in mainmenu.py.

At present, the menu functions are scattered around this file. Right now, they should be at least gathered together in one section with one class-level block comment. Or maybe the added comment should be in mainmenu.py. This should be a separate PR.

Personally, I don't like adding such comments to the code.

Such comments should have initials and at least a year. But yes, they can hang around for decades. In this case I agree, as a separate method is needed to group together menu methods. Understanding this code requires knowing the menu term definitions.

In this doc-string, I find the added description of how the method works entirely unnecessary, because it is already similarly detailed in comments.

Agreed.

This style of describing args in doc-strings is non-standard. I think it would be shorter and simpler to just remove the "Args:" heading, and change the "Returns:" section into a single line of text.

Likewise for other methods.

Agreed. Cheryl did this PR after we refactored configdialog. For that, we had some exceptionally wordy docstrings to keep things clear. This docstring should start with 'Return...'

How does unbinding the event allow the code context to do anything?

Also this is surprising to find given the title of this PR. If kept, it should be mentioned in the description and in the final commit message as an additional fix.

Agreed that revising _close, which has been somewhat problematical, is a separate issue.

Shouldn't the menu and sub-menu names in the examples be stings?

This should not be here. See previous comment about explaining menu terms once.

Isn't it overly restrictive to require a GUI for all tests here? As an extreme example, testing prepstr() certainly doesn't require a GUI.

Agreed. I might be tempted to have separate test_editor_gui and test_editor_nogui files if not for the problem of coverage measurement. In any case, refactoring could isolate non-gui code for easier testing.

If see this assertion and the next one as testing the implementation rather the behavior, so I suggest removing them.

I will check whether these should merely be deleted or replace with a behavior test.

Do I understand correctly that this uses a mock mostly to avoid all of the setup in EditorWindow.init()? That could also be accomplished by patching that method (e.g. with mock.patch) or by creating a sub-class.

We have elsewhere used a standalone class. In this case, a subclass would inherit menu_spec. I will look at how w is used.

Ping, @csabella?

I am going to first edit the new docstrings as indicated in my reponses to Tal's comments, and make a separate PR.

The problem goes deeper. There should be separate window-with-menu, frame, and text classes, and possibly an IDLE-menu class with a docstring that explains the menu system. And it maybe should be in mainmenu.py.

At present, the menu functions are scattered around this file. Right now, they should be at least gathered together in one section with one class-level block comment. Or maybe the added comment should be in mainmenu.py. This should be a separate PR.

Or leave out 'Note:'. I am going to revise these added docstrings.

Such comments should have initials and at least a year. But yes, they can hang around for decades. In this case I agree, as a separate method is needed to group together menu methods. Understanding this code requires knowing the menu term definitions.

Agreed.

Agreed. Cheryl did this PR after we refactored configdialog. For that, we had some exceptionally wordy docstrings to keep things clear. This docstring should start with 'Return...'

The first 2 added comments are helpful, the rest are obvious.

Done, part of the merge conflict.

Agreed. I might be tempted to have separate test_editor_gui and test_editor_nogui files if not for the problem of coverage measurement. In any case, refactoring could isolate non-gui code for easier testing.

We have elsewhere used a standalone class. In this case, a subclass would inherit menu_spec. I will look at how w is used.

I will check whether these should merely be deleted or replace with a behavior test.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Whoops, by responding to Tal's comments as part of a new review, my responses got listed twice, once under Tal's comment where they below and then again separately, without the context.

I minimally resolved the merge conflict in the web box so I could proceed. This did a full merge update rather than just updating the one file. There is still the redundancy of global requires gui and class requires gui to be fixed, and Tal's other conflicts. Since I had to do this, I will go ahead and edit everything on this PR.

More tk. prefixes.

I stopped the Windows test failures by commenting out the failing lines with an explanation.

The Ubuntu failure is

  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/idlelib/idle_test/test_editor.py", line 422, in test__extra_help_callback_not_windows
    ehc = partial(w._EditorWindow__extra_help_callback, w)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
AttributeError: Mock object has no attribute '_EditorWindow__extra_help_callback'. Did you mean: '_EditorWindow__recent_file_callback'?

May do the same here.

I think editor.py is more or less commit ready. Tal, review again or not as you wish.

Test_editor is not ready. test_editor passes, but still needs requires('gui') restricted. I want to try Tal's suggested alternative. And also make sure that every test is actually testing. The test I revised was not, as the function supposedly being tested was mocked.

test_mainmenu passes by itself, but test_idle fails with

File "f:\dev\3x\lib\idlelib\idle_test\test_mainmenu.py", line 17, in test_default_keydefs
self.assertGreaterEqual(len(mainmenu.default_keydefs), 50)
AssertionError: 5 not greater than or equal to 50

5 happens to be the length of keydefs used in test_editor, which run first, so the test change is somehow affecting mainmenu. This is probably via changing something in config. test_editor does not import config, but editor does.

Additional test failure: Windows error box titled "Document Start Failure" with message "(X) boom". Test freezes until box is dismissed. I suspect a test of additional help resources needs mocks for both Windows and *nix.

Commit extracted from PR python#3669. Will edits more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

Commit extracted from PR #3669. Will edit more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

…ythonGH-104446) Commit extracted from PR pythonGH-3669. Will edit more later. (cherry picked from commit 46f1c78) Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

…H-104446) (#104450) gh-75710: IDLE - add docstrings and comments to editor module (GH-104446) Commit extracted from PR GH-3669. Will edit more later. (cherry picked from commit 46f1c78) Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

This PR is stale because it has been open for 30 days with no activity.

csabella added 2 commits September 20, 2017 08:57

IDLE: Add docstrings and tests for editor.py

c21d679

Add blurb

d047c39

the-knights-who-say-ni added the CLA signed label Sep 20, 2017

bedevere-bot added the awaiting review label Sep 20, 2017

terryjreedy reviewed Sep 21, 2017

View reviewed changes

csabella mentioned this pull request Jan 20, 2019

gh-79951: IDLE - Convert menudefs to dictionary #11615

Open

taleinat reviewed Aug 9, 2019

View reviewed changes

terryjreedy requested changes Sep 21, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 21, 2020

Merge branch 'master' into binding_tests

435ea82

terryjreedy added 3 commits September 20, 2020 21:13

Synchronize tkinter import in test_editor

ac8dae4

Update test_editor.py

eb25140

More tk. prefixes.

Remove test of removed _filename_to_unicode method.

8dfbd49

terryjreedy added 3 commits September 20, 2020 23:44

Remove unneeded extra underscore.

0308f80

Make test pass; delete redundant asserts.

c32379b

Docstring and comment and a couple code revisions.

ba274a4

terryjreedy added 2 commits January 25, 2021 07:37

Merge branch 'master' into binding_tests

b5d80cb

Merge remote-tracking branch 'upstream/master' into pr_3669

3fbf6da

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 23, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022

github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 1, 2022

hauntsaninja changed the title bpo-31529: IDLE: Add docstrings and tests to some editor.py functions gh-75710: IDLE: Add docstrings and tests to some editor.py functions Jan 7, 2023

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jan 8, 2023

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 7, 2023

Merge remote-tracking branch 'upstream/main' into pr_3669

0aea634

python deleted a comment from github-actions Bot May 13, 2023

whitespace

cf8044e

terryjreedy added a commit to terryjreedy/cpython that referenced this pull request May 13, 2023

pythongh-75710 : IDLE - add docstrings and comments to editor module

4d47692

Commit extracted from PR python#3669. Will edits more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

terryjreedy mentioned this pull request May 13, 2023

gh-75710: IDLE - add docstrings and comments to editor module #104446

Merged

csabella mentioned this pull request Jul 27, 2022

IDLE: Add docstrings and tests for editor.py reload functions #75710

Open

Merge branch 'main' into binding_tests

fba2ef2

terryjreedy added a commit that referenced this pull request May 13, 2023

gh-75710 : IDLE - add docstrings and comments to editor module (#104446 )

46f1c78

Commit extracted from PR #3669. Will edit more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

miss-islington mentioned this pull request May 13, 2023

[3.11] gh-75710: IDLE - add docstrings and comments to editor module (GH-104446) #104450

Merged

Merge branch 'main' into binding_tests

47ebc2d

github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 14, 2023

github-actions Bot added the stale Stale PR or inactive for long period of time. label Jun 14, 2023

hugovk changed the title gh-75710: IDLE: Add docstrings and tests to some editor.py functions gh-75710: IDLE: Add tests to some editor.py functions Jan 9, 2025

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jan 9, 2025

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 8, 2025

                               self.apply_bindings(xkeydefs)
-                      #update menu accelerators
+                      # Update menu accelerators.
+                      # XXX - split into its own function and call it from here?

                       if self.color:
                           self.color.close(False)
                           self.color = None
+                      # Allow code context to close its text.after calls.

+                      Args:
+                          menudefs: Menu and submenu names, underlines (shortcuts),
+                              and events which is a list of tuples of the form:
+                              [(menu1, [(submenu1a, '<<virtual event>>'),

+                      eq(w.menudict['file'].entrycget(3, 'label'), 'Recent Files')
+                      # No items added to helpmenu, so the length has no value.
+                      eq(w.base_helpmenu_length, None)
+                      w.fill_menus.assert_called_with()

+                      # EditorWindow that create the menubar and submenus.
+                      # The class is mocked in order to prevent the functions
+                      # from being called automatically.
+                      w = cls.mock_editwin = mock.Mock(editor.EditorWindow)

+                      This should be called before the keybindings are applied
+                      in ApplyKeyBindings() otherwise the old bindings will still exist.
+                      Note: this does not remove the Tk/Tcl keybindings attached to

                   def createmenubar(self):
+                      """Populate the menu bar widget for the editor window.
+                      Each option on the menubar is itself a cascade-type Menu widget

-                      "Update the additional help entries on the Help menu"
+                      """Update the additional help entries on the Help menu.
+                      First the existing additional help entries are removed from

-                      "Create a callback with the helpfile value frozen at definition time"
+                      """Create a callback with the helpfile value frozen at definition time.
+                      Args:

+              def setUpModule():
+                  global root, editwin
+                  requires('gui')

                   s = re.sub(r"-[a-z]\b", lambda m: m.group().upper(), s)
+                  # Convert certain keynames to their symbol.
                   s = re.sub(r"\b\w+\b", lambda m: keynames.get(m.group(), m.group()), s)
+                  # Remove Key- from string.

		@@ -1,14 +1,432 @@
		""" Test idlelib.editor.
		"""

		@@ -1,14 +1,432 @@
		""" Test idlelib.editor.
		"""

gh-75710: IDLE: Add tests to some editor.py functions by csabella · Pull Request #3669 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-75710: IDLE: Add tests to some editor.py functions - #3669

gh-75710: IDLE: Add tests to some editor.py functions#3669csabella wants to merge 15 commits intopython:mainpython/cpython:mainfrom csabella:binding_testscsabella/cpython:binding_testsCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

csabella commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat commented Sep 20, 2020

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-75710: IDLE: Add tests to some editor.py functions#3669
csabella wants to merge 15 commits into
python:mainfrom
csabella:binding_tests

csabella commented Sep 20, 2017 •
edited by bedevere-bot
Loading

csabella commented Sep 20, 2017 •
edited
Loading

terryjreedy commented Sep 21, 2020 •
edited
Loading