◐ Shell
reader mode source ↗
Skip to content

bpo-30917: IDLE: Add config.IdleConf unittest#2691

Merged
terryjreedy merged 22 commits into
python:masterfrom
mlouielu:add_idleconf_unittest
Jul 18, 2017
Merged

bpo-30917: IDLE: Add config.IdleConf unittest#2691
terryjreedy merged 22 commits into
python:masterfrom
mlouielu:add_idleconf_unittest

Conversation

@mlouielu

@mlouielu mlouielu commented Jul 13, 2017

Copy link
Copy Markdown
Contributor

After this merged, I would like to cleanup config source code.

@mention-bot

mention-bot commented Jul 13, 2017

Copy link
Copy Markdown

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

@mlouielu mlouielu changed the title [WIP] bpo-30917: IDLE: Add config.IdleConf unittest Jul 13, 2017
@mlouielu

mlouielu commented Jul 13, 2017

Copy link
Copy Markdown
Contributor Author

@terryjreedy Could you provide the result of GetUserCfgDir() on Windows? It seems that hard code the resulting path will get undesired compare in Windows.

terryjreedy
terryjreedy requested changes Jul 15, 2017
View reviewed changes

@terryjreedy terryjreedy left a comment •
edited
Loading

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

Revert or undo 9fc7670. I will add a section on coverage to idle_test/README.txt.
bpo-30934, pr_2711

@terryjreedy

terryjreedy commented Jul 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

You were right. My machine and appveyor:

FAIL: test_get_user_cfg_dir (main.IdleConfTest)
Test to get user config directory

Traceback (most recent call last):
File "f:/dev/3x/Lib/idlelib/idle_test/test_config.py", line 221, in test_get_user_cfg_dir
self.assertEqual(conf.GetUserCfgDir(), '/home/foo/.idlerc')
AssertionError: '/home/foo\.idlerc' != '/home/foo/.idlerc'

  • /home/foo.idlerc
    ? ^
  • /home/foo/.idlerc

Adding .replace('\\', '/') in two places works on Windows. I then get warning in test output. That should be suppressed or captured.

@mlouielu

mlouielu commented Jul 15, 2017

Copy link
Copy Markdown
Contributor Author

To be mention, I also got some failed on MacOS:

.............sss.s...sss.ss....s...........s..s...s.....s..s..s........ss.........ssss..sss.......................ssss.....
======================================================================
FAIL: test_get_extension_bindings (idlelib.idle_test.test_config.IdleConfTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/louielu/Python/cpython/Lib/idlelib/idle_test/test_config.py", line 443, in test_get_extension_bindings
    {'<<zoom-height>>': ['<Alt-Key-2>']})
AssertionError: {'<<zoom-height>>': ['<Option-Key-2>']} != {'<<zoom-height>>': ['<Alt-Key-2>']}
- {'<<zoom-height>>': ['<Option-Key-2>']}
?                        ^^ ---

+ {'<<zoom-height>>': ['<Alt-Key-2>']}
?                        ^^


======================================================================
FAIL: test_get_extension_keys (idlelib.idle_test.test_config.IdleConfTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/louielu/Python/cpython/Lib/idlelib/idle_test/test_config.py", line 435, in test_get_extension_keys
    {'<<zoom-height>>': ['<Alt-Key-2>']})
AssertionError: {'<<zoom-height>>': ['<Option-Key-2>']} != {'<<zoom-height>>': ['<Alt-Key-2>']}
- {'<<zoom-height>>': ['<Option-Key-2>']}
?                        ^^ ---

+ {'<<zoom-height>>': ['<Alt-Key-2>']}
?                        ^^


----------------------------------------------------------------------

terryjreedy
terryjreedy requested changes Jul 15, 2017
View reviewed changes

@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

I will push the exact changes requested so far.

@terryjreedy

terryjreedy commented Jul 15, 2017

Copy link
Copy Markdown
Member

Since you pushed conflicting comments, I will edit the comments. to show what I did.

terryjreedy
terryjreedy reviewed Jul 15, 2017
View reviewed changes

@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

The part from where I left off looks pretty good.

@mlouielu

mlouielu commented Jul 17, 2017

Copy link
Copy Markdown
Contributor Author

@csabella would you like to help for reviewing this?

30 hidden items Load more…
csabella
csabella reviewed Jul 18, 2017
View reviewed changes

@csabella csabella left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

You did a massive amount of work for this and it's overwhelming to look at. Very impressive. Overall I thought it looks great, but I'm new to unittests, so I think Terry's critical eye will be much more helpful.

I didn't see calls for GetKeyBinding or SaveUserCfgFiles. Since you already have a test for userCfg,Save(), then maybe that could be mocked and just tested that it gets called?

@mlouielu

mlouielu commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

Thanks @csabella, your review helps a lot. I've addressed your point and update the commit. test of GetKeyBinding is missed and now updated, SaveUserCfgFiles I mock out the Save now.

terryjreedy
terryjreedy approved these changes Jul 18, 2017
View reviewed changes

@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

As Cheryl said, this patch is more than enough to comfortably review. As it is, it is a big improvement, adding 50% to coverage.

I just pushed the two changes I consider essential: suppressing printed warnings (I had the one I made conditional on 'not idlelib.testing'); and updating coverage. All the other suggestions I consider optional for this patch. I plan to push it, with or without additional commits when I can get back to it tomorrow.

@terryjreedy terryjreedy merged commit f776eb0 into python:master Jul 18, 2017
terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Jul 18, 2017
@mlouielu @terryjreedy
[3.6] bpo-30917: IDLE: Add config.IdleConf unittests (pythonGH-2691) …
1cd9efe 
Patch by Louie Lu.
(cherry picked from commit f776eb0)
@mlouielu mlouielu deleted the add_idleconf_unittest branch July 19, 2017 01:02
terryjreedy added a commit that referenced this pull request Jul 20, 2017
@terryjreedy
[3.6] bpo-30917: IDLE: Add config.IdleConf unittests (GH-2691) (#2753) …
65c24c8 
Patch by Louie Lu.
(cherry picked from commit f776eb0)
(includes diffs of  ed014f7 and 9f9192a)
* fix config reset from pr 2754

* Fix test_get_font (from pr 2769)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy terryjreedy approved these changes

@csabella csabella csabella left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mlouielu @mention-bot @terryjreedy @csabella @the-knights-who-say-ni