From 9bdacc772032bbf5f034d2f58960ae1894fe13b7 Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Tue, 11 Jul 2017 14:13:16 +0800
Subject: [PATCH 1/9] bpo-30899: IDLE: Add idle config parser unittest

---
 Lib/idlelib/idle_test/test_config.py | 151 +++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index e157bbb5b52c46..39c4fb6d41c77c 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -3,6 +3,9 @@
 Much is tested by opening config dialog live or in test_configdialog.
 Coverage: 27%
 '''
+import os
+import tempfile
+from test import support
 from test.support import captured_stderr
 import unittest
 from idlelib import config
@@ -26,6 +29,154 @@ def tearDownModule():
     idleConf.userCfg = usercfg
 
 
+class IdleConfParserTest(unittest.TestCase):
+    """Test IdleConfParser works"""
+
+    config = """
+        [one]
+        one = false
+        two = true
+        three = 10
+
+        [two]
+        one = a string
+        two = true
+        three = false
+    """
+
+    def test_get(self):
+        parser = config.IdleConfParser('')
+        parser.read_string(self.config)
+
+        self.assertEqual(parser.Get('one', 'one', type='bool'), False)
+        self.assertEqual(parser.Get('one', 'two', type='bool'), True)
+        self.assertEqual(parser.Get('one', 'three', type='int'), 10)
+        self.assertEqual(parser.Get('two', 'one'), 'a string')
+        self.assertEqual(parser.Get('two', 'two', type='bool'), True)
+        self.assertEqual(parser.Get('two', 'three', type='bool'), False)
+        self.assertEqual(parser.Get('two', 'two'), 'true')
+        self.assertEqual(parser.Get('two', 'three'), 'false')
+        self.assertEqual(parser.Get('not', 'exist'), None)
+        self.assertEqual(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')
+
+    def test_get_option_list(self):
+        parser = config.IdleConfParser('')
+        parser.read_string(self.config)
+
+        self.assertEqual(parser.GetOptionList('one'), ['one', 'two', 'three'])
+        self.assertEqual(parser.GetOptionList('two'), ['one', 'two', 'three'])
+        self.assertEqual(parser.GetOptionList('not exist'), [])
+
+    def test_load_file(self):
+        config_path = support.findfile('cfgparser.1')
+        parser = config.IdleConfParser(config_path)
+        parser.Load()
+
+        self.assertEqual(parser.Get('Foo Bar', 'foo'), 'newbar')
+        self.assertEqual(parser.GetOptionList('Foo Bar'), ['foo'])
+
+
+class IdleUserConfParserTest(unittest.TestCase):
+    """Test IdleUserConfParser works"""
+
+    def new_parser(self, path=''):
+        return config.IdleUserConfParser(path)
+
+    def test_add_section(self):
+        parser = self.new_parser()
+        self.assertEqual(parser.sections(), [])
+
+        # Duplicate section should only add one time.
+        # In normal configparser, it will raise DuplicateError,
+        # IdleParser won't raise it
+        parser.AddSection('Foo')
+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        s = parser.sections()
+        s.sort()
+        self.assertEqual(s, ['Bar', 'Foo'])
+
+    def test_remove_empty_sections(self):
+        parser = self.new_parser()
+
+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        self.assertEqual(sorted(parser.sections()), ['Bar', 'Foo'])
+        parser.RemoveEmptySections()
+        self.assertEqual(parser.sections(), [])
+
+    def test_is_empty(self):
+        parser = self.new_parser()
+
+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        self.assertEqual(parser.IsEmpty(), True)
+        self.assertEqual(parser.sections(), [])
+
+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        parser.SetOption('Foo', 'bar', 'false')
+        self.assertEqual(parser.IsEmpty(), False)
+        self.assertEqual(parser.sections(), ['Foo'])
+
+    def test_set_options(self):
+        parser = self.new_parser()
+
+        parser.AddSection('Foo')
+
+        # Set option success should return True
+        self.assertEqual(parser.SetOption('Foo', 'bar', 'true'), True)
+
+        # Set duplicate option (with same value) should return False
+        self.assertEqual(parser.SetOption('Foo', 'bar', 'true'), False)
+
+        # Set option and change value should return True
+        self.assertEqual(parser.SetOption('Foo', 'bar', 'false'), True)
+
+        # Set option to not exist section should create section and return True
+        self.assertEqual(parser.SetOption('Bar', 'bar', 'true'), True)
+        self.assertEqual(sorted(parser.sections()), ['Bar', 'Foo'])
+
+    def test_remove_options(self):
+        parser = self.new_parser()
+
+        parser.AddSection('Foo')
+        parser.SetOption('Foo', 'bar', 'true')
+
+        self.assertEqual(parser.RemoveOption('Foo', 'bar'), True)
+        self.assertEqual(parser.RemoveOption('Foo', 'bar'), False)
+        self.assertEqual(parser.RemoveOption('Not', 'Exist'), False)
+
+    def test_remove_file(self):
+        with tempfile.TemporaryDirectory() as tdir:
+            path = os.path.join(tdir, 'test.cfg')
+            parser = self.new_parser(path)
+            parser.AddSection('Foo')
+            parser.SetOption('Foo', 'bar', 'true')
+
+            parser.Save()
+            self.assertTrue(os.path.exists(path))
+            parser.RemoveFile()
+            self.assertFalse(os.path.exists(path))
+
+    def test_save(self):
+        with tempfile.TemporaryDirectory() as tdir:
+            path = os.path.join(tdir, 'test.cfg')
+            parser = self.new_parser(path)
+            parser.AddSection('Foo')
+            parser.SetOption('Foo', 'bar', 'true')
+
+            # Should save to path when config is not empty
+            self.assertFalse(os.path.exists(path))
+            parser.Save()
+            self.assertTrue(os.path.exists(path))
+
+            # Should remove when config is empty
+            parser.remove_section('Foo')
+            parser.Save()
+            self.assertFalse(os.path.exists(path))
+
+
 class CurrentColorKeysTest(unittest.TestCase):
     """ Test colorkeys function with user config [Theme] and [Keys] patterns.
 

From 65ef7315d3c6e4c09884e66d29dfc80f50f7d556 Mon Sep 17 00:00:00 2001
From: Terry Jan Reedy <tjreedy@udel.edu>
Date: Tue, 11 Jul 2017 02:26:38 -0400
Subject: [PATCH 2/9] News blurb

---
 Misc/NEWS.d/next/IDLE/2017-07-11-02-26-17.bpo-30899.SQmVO8.rst | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 Misc/NEWS.d/next/IDLE/2017-07-11-02-26-17.bpo-30899.SQmVO8.rst

diff --git a/Misc/NEWS.d/next/IDLE/2017-07-11-02-26-17.bpo-30899.SQmVO8.rst b/Misc/NEWS.d/next/IDLE/2017-07-11-02-26-17.bpo-30899.SQmVO8.rst
new file mode 100644
index 00000000000000..665c7cd5bbf86b
--- /dev/null
+++ b/Misc/NEWS.d/next/IDLE/2017-07-11-02-26-17.bpo-30899.SQmVO8.rst
@@ -0,0 +1 @@
+IDLE: Add tests for ConfigParser subclasses in config. Patch by Louie Lu.

From 2527171721cd619f7f1d705f56967251e480cc53 Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Tue, 11 Jul 2017 15:20:17 +0800
Subject: [PATCH 3/9] Addressed Terry's requests

---
 Lib/idlelib/idle_test/test_config.py | 33 +++++++++++++++++-----------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index 39c4fb6d41c77c..4ab9d4c3bb7ace 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -5,8 +5,7 @@
 '''
 import os
 import tempfile
-from test import support
-from test.support import captured_stderr
+from test.support import captured_stderr, findfile
 import unittest
 from idlelib import config
 
@@ -48,16 +47,23 @@ def test_get(self):
         parser = config.IdleConfParser('')
         parser.read_string(self.config)
 
-        self.assertEqual(parser.Get('one', 'one', type='bool'), False)
-        self.assertEqual(parser.Get('one', 'two', type='bool'), True)
-        self.assertEqual(parser.Get('one', 'three', type='int'), 10)
-        self.assertEqual(parser.Get('two', 'one'), 'a string')
-        self.assertEqual(parser.Get('two', 'two', type='bool'), True)
-        self.assertEqual(parser.Get('two', 'three', type='bool'), False)
-        self.assertEqual(parser.Get('two', 'two'), 'true')
-        self.assertEqual(parser.Get('two', 'three'), 'false')
-        self.assertEqual(parser.Get('not', 'exist'), None)
-        self.assertEqual(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')
+        eq = self.assertEqual
+
+        # Test with type
+        eq(parser.Get('one', 'one', type='bool'), False)
+        eq(parser.Get('one', 'two', type='bool'), True)
+        eq(parser.Get('one', 'three', type='int'), 10)
+        eq(parser.Get('two', 'one'), 'a string')
+        eq(parser.Get('two', 'two', type='bool'), True)
+        eq(parser.Get('two', 'three', type='bool'), False)
+
+        # Test without type should fallback to string
+        eq(parser.Get('two', 'two'), 'true')
+        eq(parser.Get('two', 'three'), 'false')
+
+        # If option not exist, should return None, or default
+        eq(parser.Get('not', 'exist'), None)
+        eq(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')
 
     def test_get_option_list(self):
         parser = config.IdleConfParser('')
@@ -68,7 +74,8 @@ def test_get_option_list(self):
         self.assertEqual(parser.GetOptionList('not exist'), [])
 
     def test_load_file(self):
-        config_path = support.findfile('cfgparser.1')
+        # Test configfile 'cfgparser.1' borrow from test_configparser
+        config_path = findfile('cfgparser.1')
         parser = config.IdleConfParser(config_path)
         parser.Load()
 

From 5035a2009b5817694b2cbb4f1c5046515569045e Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Tue, 11 Jul 2017 16:55:10 +0800
Subject: [PATCH 4/9] Addressed Serhiy's comments

---
 Lib/idlelib/idle_test/test_config.py | 42 +++++++++++++---------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index 4ab9d4c3bb7ace..ee9ecd1d29138d 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -50,27 +50,27 @@ def test_get(self):
         eq = self.assertEqual
 
         # Test with type
-        eq(parser.Get('one', 'one', type='bool'), False)
-        eq(parser.Get('one', 'two', type='bool'), True)
+        self.assertIs(parser.Get('one', 'one', type='bool'), False)
+        self.assertIs(parser.Get('one', 'two', type='bool'), True)
         eq(parser.Get('one', 'three', type='int'), 10)
         eq(parser.Get('two', 'one'), 'a string')
-        eq(parser.Get('two', 'two', type='bool'), True)
-        eq(parser.Get('two', 'three', type='bool'), False)
+        self.assertIs(parser.Get('two', 'two', type='bool'), True)
+        self.assertIs(parser.Get('two', 'three', type='bool'), False)
 
         # Test without type should fallback to string
         eq(parser.Get('two', 'two'), 'true')
         eq(parser.Get('two', 'three'), 'false')
 
         # If option not exist, should return None, or default
-        eq(parser.Get('not', 'exist'), None)
+        self.assertIsNone(parser.Get('not', 'exist'))
         eq(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')
 
     def test_get_option_list(self):
         parser = config.IdleConfParser('')
         parser.read_string(self.config)
 
-        self.assertEqual(parser.GetOptionList('one'), ['one', 'two', 'three'])
-        self.assertEqual(parser.GetOptionList('two'), ['one', 'two', 'three'])
+        self.assertCountEqual(parser.GetOptionList('one'), ['one', 'two', 'three'])
+        self.assertCountEqual(parser.GetOptionList('two'), ['one', 'two', 'three'])
         self.assertEqual(parser.GetOptionList('not exist'), [])
 
     def test_load_file(self):
@@ -99,16 +99,14 @@ def test_add_section(self):
         parser.AddSection('Foo')
         parser.AddSection('Foo')
         parser.AddSection('Bar')
-        s = parser.sections()
-        s.sort()
-        self.assertEqual(s, ['Bar', 'Foo'])
+        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
 
     def test_remove_empty_sections(self):
         parser = self.new_parser()
 
         parser.AddSection('Foo')
         parser.AddSection('Bar')
-        self.assertEqual(sorted(parser.sections()), ['Bar', 'Foo'])
+        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
         parser.RemoveEmptySections()
         self.assertEqual(parser.sections(), [])
 
@@ -117,14 +115,14 @@ def test_is_empty(self):
 
         parser.AddSection('Foo')
         parser.AddSection('Bar')
-        self.assertEqual(parser.IsEmpty(), True)
+        self.assertTrue(parser.IsEmpty())
         self.assertEqual(parser.sections(), [])
 
         parser.AddSection('Foo')
         parser.AddSection('Bar')
         parser.SetOption('Foo', 'bar', 'false')
-        self.assertEqual(parser.IsEmpty(), False)
-        self.assertEqual(parser.sections(), ['Foo'])
+        self.assertFalse(parser.IsEmpty())
+        self.assertCountEqual(parser.sections(), ['Foo'])
 
     def test_set_options(self):
         parser = self.new_parser()
@@ -132,17 +130,17 @@ def test_set_options(self):
         parser.AddSection('Foo')
 
         # Set option success should return True
-        self.assertEqual(parser.SetOption('Foo', 'bar', 'true'), True)
+        self.assertTrue(parser.SetOption('Foo', 'bar', 'true'))
 
         # Set duplicate option (with same value) should return False
-        self.assertEqual(parser.SetOption('Foo', 'bar', 'true'), False)
+        self.assertFalse(parser.SetOption('Foo', 'bar', 'true'))
 
         # Set option and change value should return True
-        self.assertEqual(parser.SetOption('Foo', 'bar', 'false'), True)
+        self.assertTrue(parser.SetOption('Foo', 'bar', 'false'))
 
         # Set option to not exist section should create section and return True
-        self.assertEqual(parser.SetOption('Bar', 'bar', 'true'), True)
-        self.assertEqual(sorted(parser.sections()), ['Bar', 'Foo'])
+        self.assertTrue(parser.SetOption('Bar', 'bar', 'true'))
+        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
 
     def test_remove_options(self):
         parser = self.new_parser()
@@ -150,9 +148,9 @@ def test_remove_options(self):
         parser.AddSection('Foo')
         parser.SetOption('Foo', 'bar', 'true')
 
-        self.assertEqual(parser.RemoveOption('Foo', 'bar'), True)
-        self.assertEqual(parser.RemoveOption('Foo', 'bar'), False)
-        self.assertEqual(parser.RemoveOption('Not', 'Exist'), False)
+        self.assertTrue(parser.RemoveOption('Foo', 'bar'))
+        self.assertFalse(parser.RemoveOption('Foo', 'bar'))
+        self.assertFalse(parser.RemoveOption('Not', 'Exist'))
 
     def test_remove_file(self):
         with tempfile.TemporaryDirectory() as tdir:

From 019402c51e3667b33c4af898f5b5555124777981 Mon Sep 17 00:00:00 2001
From: Terry Jan Reedy <tjreedy@udel.edu>
Date: Tue, 11 Jul 2017 22:59:47 -0400
Subject: [PATCH 5/9] Reorder some functions, add tests, fix nitpicks.

---
 Lib/idlelib/config.py                |  54 +++++++-------
 Lib/idlelib/idle_test/test_config.py | 107 ++++++++++++++-------------
 Lib/test/cfgparser.1                 |   1 +
 3 files changed, 85 insertions(+), 77 deletions(-)

diff --git a/Lib/idlelib/config.py b/Lib/idlelib/config.py
index ed37f11a9cb8e0..30bbc2ff1f376a 100644
--- a/Lib/idlelib/config.py
+++ b/Lib/idlelib/config.py
@@ -81,31 +81,6 @@ class IdleUserConfParser(IdleConfParser):
     IdleConfigParser specialised for user configuration handling.
     """
 
-    def AddSection(self, section):
-        "If section doesn't exist, add it."
-        if not self.has_section(section):
-            self.add_section(section)
-
-    def RemoveEmptySections(self):
-        "Remove any sections that have no options."
-        for section in self.sections():
-            if not self.GetOptionList(section):
-                self.remove_section(section)
-
-    def IsEmpty(self):
-        "Return True if no sections after removing empty sections."
-        self.RemoveEmptySections()
-        return not self.sections()
-
-    def RemoveOption(self, section, option):
-        """Return True if option is removed from section, else False.
-
-        False if either section does not exist or did not have option.
-        """
-        if self.has_section(section):
-            return self.remove_option(section, option)
-        return False
-
     def SetOption(self, section, option, value):
         """Return True if option is added or changed to value, else False.
 
@@ -123,6 +98,31 @@ def SetOption(self, section, option, value):
             self.set(section, option, value)
             return True
 
+    def RemoveOption(self, section, option):
+        """Return True if option is removed from section, else False.
+
+        False if either section does not exist or did not have option.
+        """
+        if self.has_section(section):
+            return self.remove_option(section, option)
+        return False
+
+    def AddSection(self, section):
+        "If section doesn't exist, add it."
+        if not self.has_section(section):
+            self.add_section(section)
+
+    def RemoveEmptySections(self):
+        "Remove any sections that have no options."
+        for section in self.sections():
+            if not self.GetOptionList(section):
+                self.remove_section(section)
+
+    def IsEmpty(self):
+        "Return True if no sections after removing empty sections."
+        self.RemoveEmptySections()
+        return not self.sections()
+
     def RemoveFile(self):
         "Remove user config file self.file from disk if it exists."
         if os.path.exists(self.file):
@@ -828,7 +828,8 @@ def save_option(config_type, section, item, value):
     def save_all(self):
         """Save configuration changes to the user config file.
 
-        Then clear self in preparation for additional changes.
+        Clear self in preparation for additional changes.
+        Return cfg_type_changed for testing.
         """
         idleConf.userCfg['main'].Save()
         for config_type in self:
@@ -849,6 +850,7 @@ def save_all(self):
         self.clear()
         # ConfigDialog caller must add the following call
         # self.save_all_changed_extensions()  # Uses a different mechanism.
+        return cfg_type_changed
 
     def delete_section(self, config_type, section):
         """Delete a section from self, userCfg, and file.
diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index ee9ecd1d29138d..ecf78ed5be8e54 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -1,7 +1,9 @@
 '''Test idlelib.config.
 
-Much is tested by opening config dialog live or in test_configdialog.
-Coverage: 27%
+Coverage: 46% (100% for IdleConfParser, IdleUserConfParser*, ConfigChanges).
+* Except is OSError clause in Save method.
+Much of IdleConf is exercised by ConfigDialog and test_configdialog,
+but it should be tested here.
 '''
 import os
 import tempfile
@@ -29,7 +31,7 @@ def tearDownModule():
 
 
 class IdleConfParserTest(unittest.TestCase):
-    """Test IdleConfParser works"""
+    """Test ththat IdleConfParser works"""
 
     config = """
         [one]
@@ -46,10 +48,9 @@ class IdleConfParserTest(unittest.TestCase):
     def test_get(self):
         parser = config.IdleConfParser('')
         parser.read_string(self.config)
-
         eq = self.assertEqual
 
-        # Test with type
+        # Test with type argument.
         self.assertIs(parser.Get('one', 'one', type='bool'), False)
         self.assertIs(parser.Get('one', 'two', type='bool'), True)
         eq(parser.Get('one', 'three', type='int'), 10)
@@ -57,24 +58,29 @@ def test_get(self):
         self.assertIs(parser.Get('two', 'two', type='bool'), True)
         self.assertIs(parser.Get('two', 'three', type='bool'), False)
 
-        # Test without type should fallback to string
+        # Test without type should fallback to string.
         eq(parser.Get('two', 'two'), 'true')
         eq(parser.Get('two', 'three'), 'false')
 
-        # If option not exist, should return None, or default
+        # If option not exist, should return None, or default.
         self.assertIsNone(parser.Get('not', 'exist'))
         eq(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')
 
     def test_get_option_list(self):
         parser = config.IdleConfParser('')
         parser.read_string(self.config)
+        get_list = parser.GetOptionList
+        self.assertCountEqual(get_list('one'), ['one', 'two', 'three'])
+        self.assertCountEqual(get_list('two'), ['one', 'two', 'three'])
+        self.assertEqual(get_list('not exist'), [])
 
-        self.assertCountEqual(parser.GetOptionList('one'), ['one', 'two', 'three'])
-        self.assertCountEqual(parser.GetOptionList('two'), ['one', 'two', 'three'])
-        self.assertEqual(parser.GetOptionList('not exist'), [])
+    def test_load_nothing(self):
+        parser = config.IdleConfParser('')
+        parser.Load()
+        self.assertEqual(parser.sections(), [])
 
     def test_load_file(self):
-        # Test configfile 'cfgparser.1' borrow from test_configparser
+        # Borrow test/cfgparser.1 from test_configparser.
         config_path = findfile('cfgparser.1')
         parser = config.IdleConfParser(config_path)
         parser.Load()
@@ -84,18 +90,42 @@ def test_load_file(self):
 
 
 class IdleUserConfParserTest(unittest.TestCase):
-    """Test IdleUserConfParser works"""
+    """Test that IdleUserConfParser works"""
 
     def new_parser(self, path=''):
         return config.IdleUserConfParser(path)
 
+    def test_set_option(self):
+        parser = self.new_parser()
+        parser.add_section('Foo')
+        # Setting new option in existing section should return True.
+        self.assertTrue(parser.SetOption('Foo', 'bar', 'true'))
+        # Setting existing option with same value should return False.
+        self.assertFalse(parser.SetOption('Foo', 'bar', 'true'))
+        # Setting exiting option with new value should return True.
+        self.assertTrue(parser.SetOption('Foo', 'bar', 'false'))
+        self.assertEqual(parser.Get('Foo', 'bar'), 'false')
+        
+        # Setting option in new section should create section and return True.
+        self.assertTrue(parser.SetOption('Bar', 'bar', 'true'))
+        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
+        self.assertEqual(parser.Get('Bar', 'bar'), 'true')
+
+    def test_remove_option(self):
+        parser = self.new_parser()
+        parser.AddSection('Foo')
+        parser.SetOption('Foo', 'bar', 'true')
+
+        self.assertTrue(parser.RemoveOption('Foo', 'bar'))
+        self.assertFalse(parser.RemoveOption('Foo', 'bar'))
+        self.assertFalse(parser.RemoveOption('Not', 'Exist'))
+
     def test_add_section(self):
         parser = self.new_parser()
         self.assertEqual(parser.sections(), [])
 
-        # Duplicate section should only add one time.
-        # In normal configparser, it will raise DuplicateError,
-        # IdleParser won't raise it
+        # Should not add duplicate section.
+        # Configparser raises DuplicateError, IdleParser not.
         parser.AddSection('Foo')
         parser.AddSection('Foo')
         parser.AddSection('Bar')
@@ -106,9 +136,10 @@ def test_remove_empty_sections(self):
 
         parser.AddSection('Foo')
         parser.AddSection('Bar')
-        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
+        parser.SetOption('Idle', 'name', 'val')
+        self.assertCountEqual(parser.sections(), ['Bar', 'Foo', 'Idle'])
         parser.RemoveEmptySections()
-        self.assertEqual(parser.sections(), [])
+        self.assertEqual(parser.sections(), ['Idle'])
 
     def test_is_empty(self):
         parser = self.new_parser()
@@ -118,47 +149,19 @@ def test_is_empty(self):
         self.assertTrue(parser.IsEmpty())
         self.assertEqual(parser.sections(), [])
 
-        parser.AddSection('Foo')
-        parser.AddSection('Bar')
         parser.SetOption('Foo', 'bar', 'false')
+        parser.AddSection('Bar')
         self.assertFalse(parser.IsEmpty())
         self.assertCountEqual(parser.sections(), ['Foo'])
 
-    def test_set_options(self):
-        parser = self.new_parser()
-
-        parser.AddSection('Foo')
-
-        # Set option success should return True
-        self.assertTrue(parser.SetOption('Foo', 'bar', 'true'))
-
-        # Set duplicate option (with same value) should return False
-        self.assertFalse(parser.SetOption('Foo', 'bar', 'true'))
-
-        # Set option and change value should return True
-        self.assertTrue(parser.SetOption('Foo', 'bar', 'false'))
-
-        # Set option to not exist section should create section and return True
-        self.assertTrue(parser.SetOption('Bar', 'bar', 'true'))
-        self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
-
-    def test_remove_options(self):
-        parser = self.new_parser()
-
-        parser.AddSection('Foo')
-        parser.SetOption('Foo', 'bar', 'true')
-
-        self.assertTrue(parser.RemoveOption('Foo', 'bar'))
-        self.assertFalse(parser.RemoveOption('Foo', 'bar'))
-        self.assertFalse(parser.RemoveOption('Not', 'Exist'))
-
     def test_remove_file(self):
         with tempfile.TemporaryDirectory() as tdir:
             path = os.path.join(tdir, 'test.cfg')
             parser = self.new_parser(path)
+            parser.RemoveFile()  # should not raise
+
             parser.AddSection('Foo')
             parser.SetOption('Foo', 'bar', 'true')
-
             parser.Save()
             self.assertTrue(os.path.exists(path))
             parser.RemoveFile()
@@ -171,12 +174,12 @@ def test_save(self):
             parser.AddSection('Foo')
             parser.SetOption('Foo', 'bar', 'true')
 
-            # Should save to path when config is not empty
+            # Should save to path when config is not empty.
             self.assertFalse(os.path.exists(path))
             parser.Save()
             self.assertTrue(os.path.exists(path))
 
-            # Should remove when config is empty
+            # Should remove when config is empty.
             parser.remove_section('Foo')
             parser.Save()
             self.assertFalse(os.path.exists(path))
@@ -335,10 +338,12 @@ def test_save_option(self):  # Static function does not touch changes.
 
     def test_save_added(self):
         changes = self.load()
-        changes.save_all()
+        assert(changes.save_all(), True)
         self.assertEqual(usermain['Msec']['mitem'], 'mval')
         self.assertEqual(userhigh['Hsec']['hitem'], 'hval')
         self.assertEqual(userkeys['Ksec']['kitem'], 'kval')
+        changes.add_option('main', 'Msec', 'mitem', 'mval')
+        assert(changes.save_all(), False)
         usermain.remove_section('Msec')
         userhigh.remove_section('Hsec')
         userkeys.remove_section('Ksec')
diff --git a/Lib/test/cfgparser.1 b/Lib/test/cfgparser.1
index 3387f52eca6f94..0c0e0d35273a39 100644
--- a/Lib/test/cfgparser.1
+++ b/Lib/test/cfgparser.1
@@ -1,2 +1,3 @@
+# Also used by idlelib.test_idle.test_config.
 [Foo Bar]
 foo=newbar

From 2a7e47891fa7b2e5bd880f6b11c67185d7f277a8 Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Wed, 12 Jul 2017 17:53:54 +0800
Subject: [PATCH 6/9] Add some comments

---
 Lib/idlelib/idle_test/test_config.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index ecf78ed5be8e54..4e23d7ca6f01b7 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -105,7 +105,7 @@ def test_set_option(self):
         # Setting exiting option with new value should return True.
         self.assertTrue(parser.SetOption('Foo', 'bar', 'false'))
         self.assertEqual(parser.Get('Foo', 'bar'), 'false')
-        
+
         # Setting option in new section should create section and return True.
         self.assertTrue(parser.SetOption('Bar', 'bar', 'true'))
         self.assertCountEqual(parser.sections(), ['Bar', 'Foo'])
@@ -158,7 +158,7 @@ def test_remove_file(self):
         with tempfile.TemporaryDirectory() as tdir:
             path = os.path.join(tdir, 'test.cfg')
             parser = self.new_parser(path)
-            parser.RemoveFile()  # should not raise
+            parser.RemoveFile()  # should not raise exception
 
             parser.AddSection('Foo')
             parser.SetOption('Foo', 'bar', 'true')
@@ -179,7 +179,7 @@ def test_save(self):
             parser.Save()
             self.assertTrue(os.path.exists(path))
 
-            # Should remove when config is empty.
+            # Should remove the file from disk when config is empty.
             parser.remove_section('Foo')
             parser.Save()
             self.assertFalse(os.path.exists(path))

From d519d34b9137549e54a90204e23eb81b36bcf6c2 Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Wed, 12 Jul 2017 17:55:26 +0800
Subject: [PATCH 7/9] Fix assert always be true

---
 Lib/idlelib/idle_test/test_config.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index 4e23d7ca6f01b7..886f8ae742411a 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -338,12 +338,12 @@ def test_save_option(self):  # Static function does not touch changes.
 
     def test_save_added(self):
         changes = self.load()
-        assert(changes.save_all(), True)
+        self.assertTrue(changes.save_all())
         self.assertEqual(usermain['Msec']['mitem'], 'mval')
         self.assertEqual(userhigh['Hsec']['hitem'], 'hval')
         self.assertEqual(userkeys['Ksec']['kitem'], 'kval')
         changes.add_option('main', 'Msec', 'mitem', 'mval')
-        assert(changes.save_all(), False)
+        self.assertFalse(changes.save_all())
         usermain.remove_section('Msec')
         userhigh.remove_section('Hsec')
         userkeys.remove_section('Ksec')

From 4fa460f3e8e1149a01798f7c7b97be8d4f0ba05e Mon Sep 17 00:00:00 2001
From: Louie Lu <git@louie.lu>
Date: Wed, 12 Jul 2017 18:14:48 +0800
Subject: [PATCH 8/9] Fix save_all return bad result

---
 Lib/idlelib/config.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Lib/idlelib/config.py b/Lib/idlelib/config.py
index 30bbc2ff1f376a..dc8e1f1c66dbaf 100644
--- a/Lib/idlelib/config.py
+++ b/Lib/idlelib/config.py
@@ -829,9 +829,11 @@ def save_all(self):
         """Save configuration changes to the user config file.
 
         Clear self in preparation for additional changes.
-        Return cfg_type_changed for testing.
+        Return changed for testing.
         """
         idleConf.userCfg['main'].Save()
+
+        changed = False
         for config_type in self:
             cfg_type_changed = False
             page = self[config_type]
@@ -844,13 +846,14 @@ def save_all(self):
                         cfg_type_changed = True
             if cfg_type_changed:
                 idleConf.userCfg[config_type].Save()
+                changed = True
         for config_type in ['keys', 'highlight']:
             # Save these even if unchanged!
             idleConf.userCfg[config_type].Save()
         self.clear()
         # ConfigDialog caller must add the following call
         # self.save_all_changed_extensions()  # Uses a different mechanism.
-        return cfg_type_changed
+        return changed
 
     def delete_section(self, config_type, section):
         """Delete a section from self, userCfg, and file.

From 6747e7c818a37e52ec8232c57d5e93e7e9f02d28 Mon Sep 17 00:00:00 2001
From: terryjreedy <tjreedy@udel.edu>
Date: Wed, 12 Jul 2017 13:44:32 -0400
Subject: [PATCH 9/9] Update test_config.py

---
 Lib/idlelib/idle_test/test_config.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Lib/idlelib/idle_test/test_config.py b/Lib/idlelib/idle_test/test_config.py
index 886f8ae742411a..5c41aff19688be 100644
--- a/Lib/idlelib/idle_test/test_config.py
+++ b/Lib/idlelib/idle_test/test_config.py
@@ -31,7 +31,7 @@ def tearDownModule():
 
 
 class IdleConfParserTest(unittest.TestCase):
-    """Test ththat IdleConfParser works"""
+    """Test that IdleConfParser works"""
 
     config = """
         [one]
@@ -158,7 +158,7 @@ def test_remove_file(self):
         with tempfile.TemporaryDirectory() as tdir:
             path = os.path.join(tdir, 'test.cfg')
             parser = self.new_parser(path)
-            parser.RemoveFile()  # should not raise exception
+            parser.RemoveFile()  # Should not raise exception.
 
             parser.AddSection('Foo')
             parser.SetOption('Foo', 'bar', 'true')