◐ Shell
reader mode source ↗
Skip to content

bpo-30870: IDLE: Add configdialog fontlist unittest#2666

Merged
terryjreedy merged 12 commits into
python:masterfrom
mlouielu:bpo-30870-fontlist-unittest
Jul 14, 2017
Merged

bpo-30870: IDLE: Add configdialog fontlist unittest#2666
terryjreedy merged 12 commits into
python:masterfrom
mlouielu:bpo-30870-fontlist-unittest

Conversation

@mlouielu

Copy link
Copy Markdown
Contributor

No description provided.

@mention-bot

Copy link
Copy Markdown

@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 terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Super. Will look at tests later.

@terryjreedy

terryjreedy commented Jul 11, 2017

Copy link
Copy Markdown
Member

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

mlouielu commented Jul 12, 2017

Copy link
Copy Markdown
Contributor Author

@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

mlouielu commented Jul 12, 2017

Copy link
Copy Markdown
Contributor Author

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

@terryjreedy

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

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

@terryjreedy

Copy link
Copy Markdown
Member

Changes:

  1. Using verb configure for the dialog was a mistake, probably mine. Corrected.
  2. 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.
  3. 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.
  4. Abbreviated 'dialog.fontlist' as 'fontlist'.
  5. Setting tk font Variables has no effect on fontlist. Deleting that block had no effect.
  6. 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

Copy link
Copy Markdown
Contributor Author

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

@terryjreedy 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
terryjreedy added a commit that referenced this pull request Jul 14, 2017
…H-2666) (#2701)

Initial patch by Louie Lu.
(cherry picked from commit 9b622fb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants