bpo-30870: IDLE: Add configdialog fontlist unittest by mlouielu · Pull Request #2666 · python/cpython

mlouielu · 2017-07-11T08:49:15Z

No description provided.

mention-bot · 2017-07-11T08:49:16Z

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

terryjreedy · 2017-07-11T09:20:03Z

Lib/idlelib/configdialog.py

        self.attach_var_callbacks()  # Avoid callbacks during load_configs.

        if not _utest:
+            self.grab_set()


I believe at least some other dialogs have grab_set here already. Is this move also needed for event_generate?

I only saw it at textview that control with model argument. grab_set somehow will affect event_generate, too.

test_textview mock out the grab_set, hmm. Should we mock out, too?

Since it will not be called, no need. I just read the UNM Shipman doc "Widget w grabs all events for w's application. If there was another grab in force, it goes away. See Section 54, “Events” for a discussion of grabs." Seems to be related to dialog being modal. Did not see anything in Sect.54 though. I need to make sure I know about all the calls in various inits.

terryjreedy

Super. Will look at tests later.

terryjreedy · 2017-07-11T09:27:01Z

Appveyor, running on Windows, and my Win 10 machine, say:

FAIL: test_key_up_down_should_change_font_and_sample (idlelib.idle_test.test_configdialog.FontTabTest)
Traceback (most recent call last):
File "C:\projects\cpython\lib\idlelib\idle_test\test_configdialog.py", line 100, in test_key_up_down_should_change_font_and_sample
self.assertNotEqual(down_font, font)
AssertionError: 'Courier New' == 'Courier New'
EDIT
These look the same to me, so this test should fail.
print('\n', [ord(c) for c in down_font], '\n', [ord(c) for c in font])
I will try to figure out why OS should make a difference, and what is going on on Windows.

mlouielu · 2017-07-12T10:30:54Z

@terryjreedy I think the assertion is correct, the result we want to pass is the down_font different with font, since down_font expected be another font.

mlouielu · 2017-07-12T10:34:30Z

After adding focus_force, it pass both on travis and appveyor.

terryjreedy · 2017-07-12T18:02:15Z

@mlouielu I initially mis-interpreted Appveyor result before I ran test myself and realized that the failure was correct and pointed to a need for a fix. I presume focus_force created no problem on your Linux.

terryjreedy · 2017-07-13T02:55:14Z

I have reviewed and am revising and will push for review and test. Please don't push any more commits for now.

terryjreedy · 2017-07-13T04:24:39Z

Changes:

Using verb configure for the dialog was a mistake, probably mine. Corrected.
Testing events and event_handler together is necessary. Trying to test set_font_sample at the same time was my mistake. I corrected this by masking the function with an instance mock that could be queried about calls. I moved these 2 tests into a new class which describes what is to be tested.
It does not matter what item on the list starts as active, so the first is as good as any. What matters is what happens after the simulated user actions.
Abbreviated 'dialog.fontlist' as 'fontlist'.
Setting tk font Variables has no effect on fontlist. Deleting that block had no effect.
The click test did not test the effect of clicking. It passed without the simulated click. The generated button press is needed to set the anchor read in the event handler. Added a test that the click actually selected the intended item.

mlouielu · 2017-07-13T04:46:52Z

Lib/idlelib/idle_test/test_configdialog.py

+        fontlist = dialog.fontlist
+        fontlist.activate(0)
+
+      # Select next item in listbox


Need two more space for the comment. I'll fixed this.

mlouielu · 2017-07-13T04:53:14Z

@terryjreedy Thanks for your help, this makes the test more reliable. I fix some nit point about styling. Otherwise LGTM.

terryjreedy · 2017-07-13T05:01:49Z

Lib/idlelib/idle_test/test_configdialog.py

-        fontlist.event_generate('<Button-1>', x=x+dx//2, y=y+dy//2)
-        fontlist.event_generate('<ButtonRelease-1>', x=x+dx//2, y=y+dy//2)
+        fontlist.event_generate('<Button-1>', x=x + dx // 2, y=y + dy // 2)
+        fontlist.event_generate('<ButtonRelease-1>', x=x + dx // 2, y=y + dy // 2)


I disagree. For an assignment statement, I would write
x = x + dx//2
Following math tradition here is allowed by pep8. When the space around '=' is deleted, as per PEP8, it looks weird, and visually deceptive, to leave space around '+'. I don't know if this is specifically addressed.

After re-reading PEP8, I concur with your way to present x = x + dx//2, but it's weird in this situation, too.

I convert it to another way to represent:

x += dx // 2 y += dy // 2 event_generate(x=y, y=y)

I hope this will be more general in this case.

With two uses, that looks better.

terryjreedy · 2017-07-13T05:04:05Z

Lib/idlelib/idle_test/test_configdialog.py

    @classmethod
    def tearDownClass(cls):
-        del dialog.set_font_sample  # unmask instance method
+        del dialog.set_font_sample  # Unmask instance method


Old habits die hard. I added the '.'s.

terryjreedy · 2017-07-13T05:28:27Z

Lib/idlelib/idle_test/test_configdialog.py


    def test_select_font_mouse(self):
-        # Click on item should select that item.
+        "Click on item should select that item."


I tried this once and stopped when I discovered that running a file with unittest.main(verbosity=2) prints the docstrings, as in this example where I added one. Ditto for python -m test.test_idle -v.
test_font (__main__.FontTabTest) ... ok
test_set_sample (__main__.FontTabTest)
Set_font_sample also sets highlight_sample. ... ok
test_tabspace (__main__.FontTabTest) ... ok
I decided I did not like it and reverted the couple of examples.

Verbosity 1 prints a '.' for each successful test. 0 prints nothing until the end.

With the effect understood, I am willing to discuss a change, with at least Cheryl included.
In the meanwhile, please revert (you can use pencil edit upper right).

OK, revert this first.

…ythonGH-2666) Initial patch by Louie Lu. (cherry picked from commit 9b622fb)

…H-2666) (#2701) Initial patch by Louie Lu. (cherry picked from commit 9b622fb)

bpo-30870: IDLE: Add configdialog fontlist unittest

ff8ab19

the-knights-who-say-ni added the CLA signed label Jul 11, 2017

terryjreedy reviewed Jul 11, 2017

View reviewed changes

Add focus_force to fontlist to prevent unittest failed

bd1c784

terryjreedy added 2 commits July 12, 2017 23:55

revise tests

5f8df49

Update test_configdialog.py

255fd75

Update test_configdialog.py

e676416

mlouielu commented Jul 13, 2017

View reviewed changes

mlouielu added 4 commits July 13, 2017 12:48

Fix nit coding style

65b9c25

Fix misused of self in classmethod

4ed55c9

Fix comment

331354d

Change comment to docstring in two test cases

961cc3d

terryjreedy reviewed Jul 13, 2017

View reviewed changes

terryjreedy and others added 2 commits July 13, 2017 01:03

Update test_configdialog.py

f43d396

Change x and y represent

6f58026

terryjreedy requested changes Jul 13, 2017

View reviewed changes

Revert changes for test cases' docstring

ec67c26

terryjreedy approved these changes Jul 14, 2017

View reviewed changes

terryjreedy merged commit 9b622fb into python:master Jul 14, 2017

terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Jul 14, 2017

[3.6] bpo-30870: IDLE: Add configdialog fontlist selection unittest (p…

9cea1d6

…ythonGH-2666) Initial patch by Louie Lu. (cherry picked from commit 9b622fb)

terryjreedy mentioned this pull request Jul 14, 2017

[3.6] bpo-30870: IDLE: Add configdialog fontlist selection unittest (… #2701

Merged

terryjreedy added a commit that referenced this pull request Jul 14, 2017

[3.6] bpo-30870: IDLE: Add configdialog fontlist selection unittest (G…

42abf7f

…H-2666) (#2701) Initial patch by Louie Lu. (cherry picked from commit 9b622fb)

Uh oh!

Conversation

mlouielu commented Jul 11, 2017

Uh oh!

mention-bot commented Jul 11, 2017

Uh oh!

terryjreedy Jul 11, 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!

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!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

terryjreedy commented Jul 12, 2017

Uh oh!

terryjreedy commented Jul 13, 2017

Uh oh!

terryjreedy commented Jul 13, 2017

Uh oh!

mlouielu Jul 13, 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 commented Jul 13, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

terryjreedy Jul 11, 2017 •

edited

Loading

terryjreedy commented Jul 11, 2017 •

edited

Loading

mlouielu commented Jul 12, 2017 •

edited

Loading

mlouielu commented Jul 12, 2017 •

edited

Loading

mlouielu Jul 13, 2017 •

edited

Loading