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

mlouielu · 2017-07-13T06:43:11Z

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

mention-bot · 2017-07-13T06:43:13Z

@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 · 2017-07-13T08:30:19Z

@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

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

terryjreedy · 2017-07-15T00:16:36Z

Lib/idlelib/config.py

@@ -900,7 +901,8 @@ def dumpCfg(cfg):
    dumpCfg(idleConf.userCfg)
    print('\nlines = ', line, ', crc = ', crc, sep='')

-if __name__ == '__main__':
+
+if __name__ == '__main__':  # pragma: no cover


This skip comes with coverage. I am reviewing now and will probably have other edits. If so, will do this too.

terryjreedy · 2017-07-15T00:16:39Z

Lib/idlelib/config.py

@@ -871,7 +871,8 @@ def clear(self):


 # TODO Revise test output, write expanded unittest
-def _dump():  # htest # (not really, but ignore in coverage)
+def _dump():  # pragma: no cover
+    # htest # (not really, but ignore in coverage)


There is no need to annotate things that coverage skips (it is now documented ;-).
Moving the # htest # annotation, which I added about a week ago, disables its effect.
The only thing skipped is the comment itself instead of the whole function.
_dump is a human test, but not specifically an htest. I added this note as a reminder for when I grep idlelib for '* htest *', and so no one else would be confused.

terryjreedy · 2017-07-15T00:31:56Z

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.

This reverts commit 9fc7670.

mlouielu · 2017-07-15T04:46:03Z

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

I will push the exact changes requested so far.

terryjreedy · 2017-07-15T03:45:53Z

Lib/idlelib/config.py

-            del(names[index])
-        return names
+        return sorted(
+            [n for n in extnNameList if not n.endswith(('_bindings', '_cfgBindings'))])


The original deleted in_place and did not sort. It was a roundabout version of

for i in reversed(range(names)): if names[i].endswith(('_bindings', '_cfgBindings')): del(names[i])

I checked that in-place and sorted are not needed. In fact, when doing cleanups, this should be a set comprehension. The using code can then use set union in place of 3 current lines.

def RemoveKeyBindNames(self, extnNameList): "Return extnNameList with keybinding section names removed." - return sorted( - [n for n in extnNameList if not n.endswith(('_bindings', '_cfgBindings'))]) + return [n for n in extnNameList + if not n.endswith(('_bindings', '_cfgBindings'))]

Was the sorted to be removed?

I will if I feel like it.

terryjreedy · 2017-07-15T04:47:45Z

Lib/idlelib/idle_test/test_config.py

+            conf.userCfg[ctype] = config.IdleUserConfParser(config_path)
+        conf.LoadCfgFiles()
+
+        return conf


The point of this is unclear just from reading. Having duplicate info in defaultCfg and userCfg is contrary to intended reality (though users can over-ride by hand editing). In general, I think userCfg should be loaded with fake data with read_string the way I did in the CurrentColorKeysTest.

Re-reading and parsing the .def files twice for each of multiple functions is needlessly slow. The files can be read once and then loaded into empty parsers. Besides being faster for reloads, not passing the actual file name prevents accidentally writing the files. I believe that idleConf never changes defaultCfg. (If true, we could in a followup isssue define a readonly IdleDefConfParser that raised on any attempt to change defaults.) If we want to change a particular default parser for a test, we should read into a particular parser in an expanded testCfg.

I did not yet make any changes based on the above.

I make this to be done at setUpClass, after that, mock_config will return a deepcopy copy for cls.conf, it should prevent the issue you said here.

terryjreedy · 2017-07-15T04:54:19Z

Lib/idlelib/idle_test/test_config.py

+
+        return conf
+
+    def test_get_user_cfg_dir(self):


I noticed some while ago that IdleConf.GetUserCfgDir is not really a method, in that it does not use the self parameter (idleConf). I have been planning to make it a module function, and with tests written, did it, and moved the test function into a new class, along with that for _warn.

I think this can be reference from ipython source code, it turyly seperate this function from a class like what we do here. https://github.com/ipython/ipython/blob/master/IPython/utils/path.py#L172

I think this will need to be discuss on mailing list or bpo. But I prefer to first finish the unittest, then do other stuff to change the behavior.

In config, rename function and move to module level.

+def get_user_directory(): + """Return a filesystem directory for storing user config files. +# etc - self.userdir = userDir = self.GetUserCfgDir() + self.userdir = userDir = get_user_directory() - def GetUserCfgDir(self): - """Return a filesystem directory for storing user config files. - # etc

In test_config

+class FunctionTest(unittest.TestCase): + "Test module functions." + + def test_get_user_directory(self): + getdir = config.get_user_directory + # etc, with getdir replacing call in code as it was. Move test_warn into same class.

terryjreedy · 2017-07-15T04:55:09Z

Lib/idlelib/idle_test/test_config.py

+
+    def test_get_user_cfg_dir(self):
+        "Test to get user config directory"
+        conf = self.new_config(_utest=True)


This was only needed to access the function.

terryjreedy · 2017-07-15T05:05:36Z

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

terryjreedy

The part from where I left off looks pretty good.

terryjreedy · 2017-07-15T05:24:31Z

Lib/idlelib/idle_test/test_config.py

+        conf = self.mock_config()
+
+        self.assertEqual(
+            conf.RemoveKeyBindNames(conf.GetSectionList('default', 'extensions')),


with the sorted call removed from the function, add .sort() here.

Is it better to add .sort() or to use assertCountEqual()?

I changed this to assertCountEqual, if we want to changed this to return sorted list, I think it should be after this.

terryjreedy · 2017-07-15T05:28:36Z

Lib/idlelib/idle_test/test_config.py

+        eq(conf.GetExtensions(),
+           ['AutoComplete', 'AutoExpand', 'CallTips', 'CodeContext',
+            'FormatParagraph', 'ParenMatch', 'RstripExtension',
+            'ScriptBinding', 'ZoomHeight', 'Foobar'])  # User extensions didn't sort


If you delete .sorted() from Remove... and add .sort() here, Foobar will be sorted in the list.
I see the point that loading user extensions with def extensions, even if unrealistic, makes it easy to test functions like this on both. This is good enough for now. I am mainly concerned about repeated disk reads.

terryjreedy · 2017-07-15T05:35:01Z

Lib/idlelib/idle_test/test_config.py

+
+        self.assertEqual(
+            conf.GetFont(root, 'main', 'EditorWindow'),
+            (f['family'], 10 if f['size'] < 10 else f['size'], f['weight']))


End function with root.destroy() so that children are gone before root disappears.

mlouielu · 2017-07-15T05:40:26Z

Lib/idlelib/idle_test/test_config.py

+        self.assertEqual(conf.GetExtensionBindings('Foobar'), {'<<foobar>>': ['<Key-F>']})
+
+    def test_get_current_keyset(self):
+        import sys


This should be removed since it is imported at the top of the test, I'll leave here until @terryjreedy push his commit to GitHub (there is a conflict here)

I not pushing anything until tomorrow my afternoon, to avoid conflict with you. (Its past midnight here.)

this has been removed.

mlouielu · 2017-07-17T07:29:00Z

@csabella would you like to help for reviewing this?

csabella · 2017-07-17T22:32:29Z

Lib/idlelib/idle_test/test_config.py

+    def test_current_colors_and_keys(self):
+        conf = self.mock_config()
+
+        self.assertEqual(conf.current_colors_and_keys('Theme'), 'IDLE Classic')


I'm wondering if there needs to be more tested here by using SetOption to change the currently active theme or keys. There's a lot in the docstring and code for just one test.

If it helps, on config dialog, there is a separate boolean for both themes and keys and defines what kind is currently selected. Meaning, if the user has selected a custom (user) theme, then the boolean is false for is_builtin_theme. If the user has selected a default keyset, then the boolean is true for are_builtin_keys. The first check for default is the test of this boolean value to see what kind of theme or keys the user has selected. Then, the rest actually gets the name of the theme (or keys) based on whether the user selected the default or not.

Sorry, just got to the other test class for this. You can probably skip my original comment. Sorry about that.

csabella · 2017-07-17T22:36:13Z

Lib/idlelib/idle_test/test_config.py

+
+    def test_default_keys(self):
+        current_platform = sys.platform
+        conf = self.new_config(_utest=True)


I had a similar test in help_about and Terry said it wasn't necessary to test each branch based on the OS. Maybe his comments don't apply here, but here's a link to the issue:
#2380

I think that is because #2380 is about bitness, that affect isn't quite huge, but this test do will affect user in the different platform (darwin using Option-Key and other using Alt-Key). So I think it will need to test this.

Cheryl, #2380 was a peculiar situation in the the correct answer depends on the machine, not the OS, and there is no automated way to get the correct answer. But we do the the correct range -- a set of two items.

While the default_keys tests mimic the current implementation, I can think of two other strategies, and ways to introduce bugs that would be detected by this test.

csabella · 2017-07-17T23:13:48Z

Lib/idlelib/idle_test/test_config.py

+           ['AutoComplete', 'AutoExpand', 'CallTips', 'CodeContext',
+            'FormatParagraph', 'ParenMatch', 'RstripExtension', 'ScriptBinding',
+            'ZoomHeight'])
+        eq(conf.GetExtensions(active_only=False),


Would it be worthwhile to set some to enable=False before running some of the tests?

Yes, thanks for pointing out. I'll put some extra test data into it.

csabella · 2017-07-17T23:29:28Z

Lib/idlelib/idle_test/test_config.py

+            self.assertEqual(conf.GetExtraHelpSourceList('bad'), [])
+        self.assertCountEqual(
+            conf.GetAllExtraHelpSourcesList(),
+            conf.GetExtraHelpSourceList('default') + conf.GetExtraHelpSourceList('user'))


Should the test for GetAllExtraHelpSourcesList() be in its own method?

I merge them into one test because if we split out this test, we will need to prepare the same data twice in the different test (test_get_extra_help_source_list and test_get_all_extrap_help_source_list). I'll add the comment on this test. Thanks for pointing out this mistake.

Testing these two functions together, one of which is so trivial (summing 2 calls), is not a mistake. The test should put something into the user config with read_string and test that GetAll returns the same thing as the single call.

csabella · 2017-07-17T23:33:53Z

Lib/idlelib/idle_test/test_config.py

+        conf.userCfg['main'].SetOption('HelpFiles', '3', 'Python:https://python.org')
+        self.assertEqual(conf.GetExtraHelpSourceList('user'),
+                         [('IDLE', 'C:/Programs/Python36/Lib/idlelib/help.html', '1'),
+                          ('Pillow', 'https://pillow.readthedocs.io/en/latest/', '2')])


The code sorts the help sources in option order before returning them. Are they stored that way? Should you add them here out of order (1=Pillow and 2-IDLE) to show that they get sorted?

Yes, docstring says that it will sort by option.

...therefore the returned list must be sorted by 'option'.

So I didn't perform another sort here and directly compare via assertEqual.

I believe Cheryl had a different, more subtle, and correct point. When number and alphabetical (codepoint) order agree, it does not matter which is used for the sort key. So a function that sorted by value, the first member of the tuple and therefore the default with no key given, instead of option, the third member, would still pass the test. Switch some pair of values.

Ah, I finally know what is talking about here, I'll take a reverse input for this cases.

csabella · 2017-07-17T23:44:32Z

Lib/idlelib/idle_test/test_config.py

+        self.assertEqual(
+            conf.GetFont(root, 'main', 'EditorWindow'),
+            (f['family'], 10 if f['size'] < 10 else f['size'], f['weight']))
+


I don't know if you saw this for TkFixedFont.
https://bugs.python.org/issue24745

Basically, the code picks the best Font for the platform instead of always using the default. I think it might be OK to mock TkFixedFont and f, but maybe it's not necessary.

csabella · 2017-07-17T23:56:42Z

Lib/idlelib/idle_test/test_config.py

+        del root
+
+    def test_get_core_keys(self):
+        conf = self.mock_config()


Did you want to mock out _warn as you did in other tests?

hmm, do you point the wrong one? I didn't get any stdout in test_get_core_keys case.

Sorry, I meant that the method for GetCoreKeys has calls to _warn, so I thought you might need to cover them.

I mocked _warn last night to prevent printed output. Adding more code to provoke calls to warn, and assert that it is called, is a good idea, and needed for complete coverage. But that can be a follow-on PR by any one of us. Reviewing such new code will be easier with this merged.

Requesting a fix for a faulty test that does not test what it claims to test is a different matter. You did well to pick up the number order = alpha order thing, which I missed.

csabella

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 · 2017-07-18T02:57:46Z

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

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 · 2017-07-18T03:42:06Z

Lib/idlelib/config.py

-            del(names[index])
-        return names
+        return sorted(
+            [n for n in extnNameList if not n.endswith(('_bindings', '_cfgBindings'))])


I will if I feel like it.

terryjreedy · 2017-07-18T04:09:43Z

Lib/idlelib/idle_test/test_config.py

+        current_platform = sys.platform
+        conf = self.new_config(_utest=True)
+
+        sys.platform = 'win'


Make this 'win32'. This is the current platform for consumer Windows regardless of bitness. If someone were to change the current implementation, 'win32' must still work.

terryjreedy · 2017-07-18T04:15:15Z

Lib/idlelib/idle_test/test_config.py

+        sys.platform = 'darwin'
+        self.assertEqual(conf.default_keys(), 'IDLE Classic OSX')
+
+        sys.platform = 'linux'


Make this 'somelinux', for similar reason to above.

terryjreedy · 2017-07-18T04:21:19Z

Lib/idlelib/idle_test/test_config.py

+
+    def test_default_keys(self):
+        current_platform = sys.platform
+        conf = self.new_config(_utest=True)


Cheryl, #2380 was a peculiar situation in the the correct answer depends on the machine, not the OS, and there is no automated way to get the correct answer. But we do the the correct range -- a set of two items.

While the default_keys tests mimic the current implementation, I can think of two other strategies, and ways to introduce bugs that would be detected by this test.

terryjreedy · 2017-07-18T04:36:04Z

Lib/idlelib/idle_test/test_config.py

+        conf.userCfg['main'].SetOption('HelpFiles', '3', 'Python:https://python.org')
+        self.assertEqual(conf.GetExtraHelpSourceList('user'),
+                         [('IDLE', 'C:/Programs/Python36/Lib/idlelib/help.html', '1'),
+                          ('Pillow', 'https://pillow.readthedocs.io/en/latest/', '2')])


I believe Cheryl had a different, more subtle, and correct point. When number and alphabetical (codepoint) order agree, it does not matter which is used for the sort key. So a function that sorted by value, the first member of the tuple and therefore the default with no key given, instead of option, the third member, would still pass the test. Switch some pair of values.

…nto add_idleconf_unittest

Patch by Louie Lu. (cherry picked from commit f776eb0)

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)

bpo-30917: IDLE: Add config.IdleConf unittest

4d90afc

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

mlouielu added 3 commits July 13, 2017 15:57

Add pragma to exclude coverage count

9fc7670

Add unittest

d4dac80

Add gui requires

eb32d91

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

terryjreedy requested changes Jul 15, 2017

View reviewed changes

Revert "Add pragma to exclude coverage count"

ddedbc7

This reverts commit 9fc7670.

mlouielu added 2 commits July 15, 2017 12:50

Add MacOS specific test correct

06d8329

Split get_user_config_dir to unix and windows

f29e0da

terryjreedy requested changes Jul 15, 2017

View reviewed changes

terryjreedy reviewed Jul 15, 2017

View reviewed changes

mlouielu commented Jul 15, 2017

View reviewed changes

mlouielu added 2 commits July 15, 2017 15:51

Remove unused import

3f2fdcc

Destroy tk.root after test cases end

e445a0e

mlouielu added 2 commits July 17, 2017 15:33

Add skipIf at get_user_cfg_dir test

0f1607d

Update mock_config to deepcopy make it faster

980a801

csabella reviewed Jul 17, 2017

View reviewed changes

csabella reviewed Jul 18, 2017

View reviewed changes

mlouielu added 4 commits July 18, 2017 10:36

Add comment explain why put extra_help_source_list in same test

4ecfb1a

Add disable extension to unittest

d9b3702

Add get_keybinding unittest

511b726

Add save_user_cfg_files unittest

49e0534

terryjreedy added 3 commits July 17, 2017 23:36

News blurb

7fba576

supress warnintg, update coverage

206ae1e

Update 2017-07-17-23-35-57.bpo-30917.hSiuuO.rst

c850b7f

terryjreedy approved these changes Jul 18, 2017

View reviewed changes

mlouielu added 4 commits July 18, 2017 15:09

Addressed terry's comments

ef45060

Put extra help source list input data reverse to check it is sorted

7627733

Merge branch 'add_idleconf_unittest' of github.com:mlouielu/cpython i…

2cbb960

…nto add_idleconf_unittest

Remove sorted in RemoveKeyBindNames

c94d4b5

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

[3.6] bpo-30917: IDLE: Add config.IdleConf unittests (pythonGH-2691)

1cd9efe

Patch by Louie Lu. (cherry picked from commit f776eb0)

mlouielu deleted the add_idleconf_unittest branch July 19, 2017 01:02

Uh oh!

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

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

Uh oh!

Conversation

mlouielu commented Jul 13, 2017

Uh oh!

mention-bot commented Jul 13, 2017

Uh oh!

mlouielu commented Jul 13, 2017

Uh oh!

terryjreedy left a comment • 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 Jul 15, 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 Jul 15, 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 Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

mlouielu commented Jul 15, 2017

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlouielu commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

terryjreedy left a comment •

edited

Loading

terryjreedy Jul 15, 2017 •

edited

Loading

terryjreedy Jul 15, 2017 •

edited

Loading

terryjreedy commented Jul 15, 2017 •

edited

Loading

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