Added some tests for the settings upgrade and fixed a bug I had introduced.

nose2.cfg

@@ -20,3 +20,8 @@ coverage-report = html
 [multiprocess]
 always-on = false
 processes = 4
+
+[output-buffer]
+always-on = True
+stderr = True
+stdout = False

openlp/core/common/settings.py

@@ -40,7 +40,7 @@ __version__ = 2
 
 # Fix for bug #1014422.
 X11_BYPASS_DEFAULT = True
-if is_linux():
+if is_linux():                                                                              # pragma: no cover
     # Default to False on Gnome.
     X11_BYPASS_DEFAULT = bool(not os.environ.get('GNOME_DESKTOP_SESSION_ID'))
     # Default to False on Xfce.
@@ -495,7 +495,7 @@ class Settings(QtCore.QSettings):
                 # system.
                 if not isinstance(old_keys, (tuple, list)):
                     old_keys = [old_keys]
-                if not any([self.contains(old_key) for old_key in old_keys]):
+                if any([not self.contains(old_key) for old_key in old_keys]):
                     log.warning('One of {} does not exist, skipping upgrade'.format(old_keys))
                     continue
                 if new_key:
@@ -521,7 +521,7 @@ class Settings(QtCore.QSettings):
                             break
                     self.setValue(new_key, new_value)
                 [self.remove(old_key) for old_key in old_keys if old_key != new_key]
-        self.setValue('settings/version', version)
+            self.setValue('settings/version', version)
 
     def value(self, key):
         """

tests/functional/openlp_core/common/test_settings.py

@@ -23,9 +23,10 @@
 Package to test the openlp.core.lib.settings package.
 """
 from unittest import TestCase
-from unittest.mock import patch
+from unittest.mock import call, patch
 
-from openlp.core.common.settings import Settings
+from openlp.core.common import settings
+from openlp.core.common.settings import Settings, media_players_conv
 
 from tests.helpers.testmixin import TestMixin
 
@@ -47,10 +48,19 @@ class TestSettings(TestCase, TestMixin):
         """
         self.destroy_settings()
 
+    def test_media_players_conv(self):
+        """Test the media players conversion function"""
+        # GIVEN: A list of media players
+        media_players = 'phonon,webkit,vlc'
+
+        # WHEN: The media converter function is called
+        result = media_players_conv(media_players)
+
+        # THEN: The list should have been converted correctly
+        assert result == 'system,webkit,vlc'
+
     def test_settings_basic(self):
-        """
+        """Test the Settings creation and its default usage"""
-        Test the Settings creation and its default usage
-        """
         # GIVEN: A new Settings setup
 
         # WHEN reading a setting for the first time
@@ -65,10 +75,28 @@ class TestSettings(TestCase, TestMixin):
         # THEN the new value is returned when re-read
         self.assertTrue(Settings().value('core/has run wizard'), 'The saved value should have been returned')
 
+    def test_set_up_default_values(self):
+        """Test that the default values are updated"""
+        # GIVEN: A Settings object with defaults
+        # WHEN: set_up_default_values() is called
+        Settings.set_up_default_values()
+
+        # THEN: The default values should have been added to the dictionary
+        assert 'advanced/default service name' in Settings.__default_settings__
+
+    def test_get_default_value(self):
+        """Test that the default value for a setting is returned"""
+        # GIVEN: A Settings class with a default value
+        Settings.__default_settings__['test/moo'] = 'baa'
+
+        # WHEN: get_default_value() is called
+        result = Settings().get_default_value('test/moo')
+
+        # THEN: The correct default value should be returned
+        assert result == 'baa'
+
     def test_settings_override(self):
-        """
+        """Test the Settings creation and its override usage"""
-        Test the Settings creation and its override usage
-        """
         # GIVEN: an override for the settings
         screen_settings = {
             'test/extend': 'very wide',
@@ -88,9 +116,7 @@ class TestSettings(TestCase, TestMixin):
         self.assertEqual('very short', Settings().value('test/extend'), 'The saved value should be returned')
 
     def test_settings_override_with_group(self):
-        """
+        """Test the Settings creation and its override usage - with groups"""
-        Test the Settings creation and its override usage - with groups
-        """
         # GIVEN: an override for the settings
         screen_settings = {
             'test/extend': 'very wide',
@@ -112,9 +138,7 @@ class TestSettings(TestCase, TestMixin):
         self.assertEqual('very short', Settings().value('test/extend'), 'The saved value should be returned')
 
     def test_settings_nonexisting(self):
-        """
+        """Test the Settings on query for non-existing value"""
-        Test the Settings on query for non-existing value
-        """
         # GIVEN: A new Settings setup
         with self.assertRaises(KeyError) as cm:
             # WHEN reading a setting that doesn't exists
@@ -124,9 +148,7 @@ class TestSettings(TestCase, TestMixin):
         self.assertEqual(str(cm.exception), "'core/does not exists'", 'We should get an exception')
 
     def test_extend_default_settings(self):
-        """
+        """Test that the extend_default_settings method extends the default settings"""
-        Test that the extend_default_settings method extends the default settings
-        """
         # GIVEN: A patched __default_settings__ dictionary
         with patch.dict(Settings.__default_settings__,
                         {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 3}, True):
@@ -138,3 +160,52 @@ class TestSettings(TestCase, TestMixin):
             self.assertEqual(
                 Settings.__default_settings__, {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 4,
                                                 'test/extended 1': 1, 'test/extended 2': 2})
+
+    @patch('openlp.core.common.settings.QtCore.QSettings.contains')
+    @patch('openlp.core.common.settings.QtCore.QSettings.value')
+    @patch('openlp.core.common.settings.QtCore.QSettings.setValue')
+    @patch('openlp.core.common.settings.QtCore.QSettings.remove')
+    def test_upgrade_single_setting(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains):
+        """Test that the upgrade mechanism for settings works correctly for single value upgrades"""
+        # GIVEN: A settings object with an upgrade step to take (99, so that we don't interfere with real ones)
+        local_settings = Settings()
+        local_settings.__setting_upgrade_99__ = [
+            ('single/value', 'single/new value', [(str, '')])
+        ]
+        settings.__version__ = 99
+        mocked_value.side_effect = [98, 10]
+        mocked_contains.return_value = True
+
+        # WHEN: upgrade_settings() is called
+        local_settings.upgrade_settings()
+
+        # THEN: The correct calls should have been made with the correct values
+        assert mocked_value.call_count == 2, 'Settings().value() should have been called twice'
+        assert mocked_value.call_args_list == [call('settings/version', 0), call('single/value')]
+        assert mocked_setValue.call_count == 2, 'Settings().setValue() should have been called twice'
+        assert mocked_setValue.call_args_list == [call('single/new value', '10'), call('settings/version', 99)]
+        mocked_contains.assert_called_once_with('single/value')
+        mocked_remove.assert_called_once_with('single/value')
+
+    @patch('openlp.core.common.settings.QtCore.QSettings.contains')
+    @patch('openlp.core.common.settings.QtCore.QSettings.value')
+    @patch('openlp.core.common.settings.QtCore.QSettings.setValue')
+    @patch('openlp.core.common.settings.QtCore.QSettings.remove')
+    def test_upgrade_multiple_one_invalid(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains):
+        """Test that the upgrade mechanism for settings works correctly for multiple values where one is invalid"""
+        # GIVEN: A settings object with an upgrade step to take
+        local_settings = Settings()
+        local_settings.__setting_upgrade_99__ = [
+            (['multiple/value 1', 'multiple/value 2'], 'single/new value', [])
+        ]
+        settings.__version__ = 99
+        mocked_value.side_effect = [98, 10]
+        mocked_contains.side_effect = [True, False]
+
+        # WHEN: upgrade_settings() is called
+        local_settings.upgrade_settings()
+
+        # THEN: The correct calls should have been made with the correct values
+        mocked_value.assert_called_once_with('settings/version', 0)
+        mocked_setValue.assert_called_once_with('settings/version', 99)
+        assert mocked_contains.call_args_list == [call('multiple/value 1'), call('multiple/value 2')]

tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py

@@ -29,10 +29,8 @@ from unittest import TestCase
 from openlp.core.ui.media.vendor.mediainfoWrapper import MediaInfoWrapper
 
 TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', 'resources', 'media'))
-
 TEST_MEDIA = [['avi_file.avi', 61495], ['mp3_file.mp3', 134426], ['mpg_file.mpg', 9404], ['mp4_file.mp4', 188336]]
 
-
 class TestMediainfoWrapper(TestCase):
 
     def test_media_length(self):