From 3140ea434d43a7ca8b4be11b26bafafc4ebdfdb8 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Nov 2017 22:06:48 -0700 Subject: [PATCH 1/6] Add support for a multi to single setting migration --- openlp/core/common/settings.py | 60 ++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/openlp/core/common/settings.py b/openlp/core/common/settings.py index a6bc549f1..6726624ef 100644 --- a/openlp/core/common/settings.py +++ b/openlp/core/common/settings.py @@ -62,6 +62,30 @@ def media_players_conv(string): return string +def upgrade_monitor(number, x_position, y_position, height, width, can_override, can_display_on_monitor): + """ + Upgrade them monitor setting from a few single entries to a composite JSON entry + + :param int number: The old monitor number + :param int x_position: The X position + :param int y_position: The Y position + :param bool can_override: Are the screen positions overridden + :param bool can_display_on_monitor: Can OpenLP display on the monitor + :returns dict: Dictionary with the new value + """ + return { + number: { + 'displayGeometry': { + 'x': x_position, + 'y': y_position, + 'height': height, + 'width': width + } + }, + 'canDisplayOnMonitor': can_display_on_monitor + } + + class Settings(QtCore.QSettings): """ Class to wrap QSettings. @@ -255,7 +279,9 @@ class Settings(QtCore.QSettings): ('core/logo file', 'core/logo file', [(str_to_path, None)]), ('presentations/last directory', 'presentations/last directory', [(str_to_path, None)]), ('images/last directory', 'images/last directory', [(str_to_path, None)]), - ('media/last directory', 'media/last directory', [(str_to_path, None)]) + ('media/last directory', 'media/last directory', [(str_to_path, None)]), + (['core/monitor', 'core/x position', 'core/y position', 'core/height', 'core/width', 'core/override', + 'core/display on monitor'], 'core/monitors', [(upgrade_monitor, [1, 0, 0, None, None, False, False])]) ] @staticmethod @@ -464,31 +490,37 @@ class Settings(QtCore.QSettings): for version in range(current_version, __version__): version += 1 upgrade_list = getattr(self, '__setting_upgrade_{version}__'.format(version=version)) - for old_key, new_key, rules in upgrade_list: + for old_keys, new_key, rules in upgrade_list: # Once removed we don't have to do this again. - Can be removed once fully switched to the versioning # system. - if not self.contains(old_key): + if not isinstance(old_keys, (tuple, list)): + old_keys = [old_keys] + if not any([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: # Get the value of the old_key. - old_value = super(Settings, self).value(old_key) + old_values = [super(Settings, self).value(old_key) for old_key in old_keys] # When we want to convert the value, we have to figure out the default value (because we cannot get # the default value from the central settings dict. if rules: - default_value = rules[0][1] - old_value = self._convert_value(old_value, default_value) + default_values = rules[0][1] + if not isinstance(default_values, (list, tuple)): + default_values = [default_values] + old_values = [self._convert_value(old_value, default_value) + for old_value, default_value in zip(old_values, default_values)] # Iterate over our rules and check what the old_value should be "converted" to. - for new, old in rules: + new_value = None + for new_rule, old_rule in rules: # If the value matches with the condition (rule), then use the provided value. This is used to # convert values. E. g. an old value 1 results in True, and 0 in False. - if callable(new): - old_value = new(old_value) - elif old == old_value: - old_value = new + if callable(new_rule): + new_value = new_rule(*old_values) + elif old_rule in old_values: + new_value = new_rule break - self.setValue(new_key, old_value) - if new_key != old_key: - self.remove(old_key) + 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) def value(self, key): From 1cb11832bdf4475690e45fa4f23648bff0541f93 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 14 Nov 2017 00:19:10 -0700 Subject: [PATCH 2/6] Added some tests for the settings upgrade and fixed a bug I had introduced. --- nose2.cfg | 5 + openlp/core/common/settings.py | 6 +- .../openlp_core/common/test_settings.py | 105 +++++++++++++++--- .../ui/media/vendor/test_mediainfoWrapper.py | 2 - 4 files changed, 96 insertions(+), 22 deletions(-) diff --git a/nose2.cfg b/nose2.cfg index 1f2e01126..ba02ed419 100644 --- a/nose2.cfg +++ b/nose2.cfg @@ -20,3 +20,8 @@ coverage-report = html [multiprocess] always-on = false processes = 4 + +[output-buffer] +always-on = True +stderr = True +stdout = False diff --git a/openlp/core/common/settings.py b/openlp/core/common/settings.py index 6726624ef..3efac81d3 100644 --- a/openlp/core/common/settings.py +++ b/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): """ diff --git a/tests/functional/openlp_core/common/test_settings.py b/tests/functional/openlp_core/common/test_settings.py index d54a6a1e1..b1f9b0a96 100644 --- a/tests/functional/openlp_core/common/test_settings.py +++ b/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')] diff --git a/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py b/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py index f8fee253d..f2fb84c34 100644 --- a/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py +++ b/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): From d05e39fc70dfd37fc801e79ea4a07b6602355c1f Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 14 Nov 2017 23:55:57 -0700 Subject: [PATCH 3/6] Fix up test after merges --- tests/functional/openlp_core/common/test_settings.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/functional/openlp_core/common/test_settings.py b/tests/functional/openlp_core/common/test_settings.py index 825057326..5d7e88fbe 100644 --- a/tests/functional/openlp_core/common/test_settings.py +++ b/tests/functional/openlp_core/common/test_settings.py @@ -115,13 +115,14 @@ class TestSettings(TestCase, TestMixin): def test_save_existing_setting(self): """Test that saving an existing setting returns the new value""" # GIVEN: An existing setting + Settings().extend_default_settings({'test/existing value': None}) Settings().setValue('test/existing value', 'old value') # WHEN a new value is saved into config - Settings().setValue('test/extend', 'new value') + Settings().setValue('test/existing value', 'new value') # THEN the new value is returned when re-read - assert Settings().value('test/extend') == 'new value', 'The saved value should be returned' + assert Settings().value('test/existing value') == 'new value', 'The saved value should be returned' def test_settings_override_with_group(self): """Test the Settings creation and its override usage - with groups""" From 8c3fd99ed52ecff8a80d4fe01d414dffca2b95d5 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 15 Nov 2017 00:03:35 -0700 Subject: [PATCH 4/6] Fix missing line --- .../openlp_core/ui/media/vendor/test_mediainfoWrapper.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py b/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py index f2fb84c34..6ec2431b9 100644 --- a/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py +++ b/tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py @@ -31,6 +31,7 @@ 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): From 923dc1436e8955be767ba758d4c41f69df284434 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 15 Nov 2017 17:19:26 -0700 Subject: [PATCH 5/6] Remove monitor migration --- openlp/core/common/settings.py | 28 +--------------------------- 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/openlp/core/common/settings.py b/openlp/core/common/settings.py index afc75dd25..225feb4e1 100644 --- a/openlp/core/common/settings.py +++ b/openlp/core/common/settings.py @@ -62,30 +62,6 @@ def media_players_conv(string): return string -def upgrade_monitor(number, x_position, y_position, height, width, can_override, can_display_on_monitor): - """ - Upgrade them monitor setting from a few single entries to a composite JSON entry - - :param int number: The old monitor number - :param int x_position: The X position - :param int y_position: The Y position - :param bool can_override: Are the screen positions overridden - :param bool can_display_on_monitor: Can OpenLP display on the monitor - :returns dict: Dictionary with the new value - """ - return { - number: { - 'displayGeometry': { - 'x': x_position, - 'y': y_position, - 'height': height, - 'width': width - } - }, - 'canDisplayOnMonitor': can_display_on_monitor - } - - class Settings(QtCore.QSettings): """ Class to wrap QSettings. @@ -283,9 +259,7 @@ class Settings(QtCore.QSettings): ('media/last directory', 'media/last directory', [(str_to_path, None)]), ('songuasge/db password', 'songusage/db password', []), ('songuasge/db hostname', 'songusage/db hostname', []), - ('songuasge/db database', 'songusage/db database', []), - (['core/monitor', 'core/x position', 'core/y position', 'core/height', 'core/width', 'core/override', - 'core/display on monitor'], 'core/monitors', [(upgrade_monitor, [1, 0, 0, None, None, False, False])]) + ('songuasge/db database', 'songusage/db database', []) ] @staticmethod From fad67f733932f10c1ca0de91fb439f05a5554135 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 15 Nov 2017 22:03:19 -0700 Subject: [PATCH 6/6] Add some more tests --- nose2.cfg | 10 +-- .../openlp_core/common/test_settings.py | 74 +++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/nose2.cfg b/nose2.cfg index ba02ed419..ae73407d7 100644 --- a/nose2.cfg +++ b/nose2.cfg @@ -1,5 +1,5 @@ [unittest] -verbose = true +verbose = True plugins = nose2.plugins.mp [log-capture] @@ -9,16 +9,16 @@ filter = -nose log-level = ERROR [test-result] -always-on = true -descriptions = true +always-on = True +descriptions = True [coverage] -always-on = true +always-on = True coverage = openlp coverage-report = html [multiprocess] -always-on = false +always-on = False processes = 4 [output-buffer] diff --git a/tests/functional/openlp_core/common/test_settings.py b/tests/functional/openlp_core/common/test_settings.py index 5d7e88fbe..ff6ee8765 100644 --- a/tests/functional/openlp_core/common/test_settings.py +++ b/tests/functional/openlp_core/common/test_settings.py @@ -22,6 +22,7 @@ """ Package to test the openlp.core.lib.settings package. """ +from pathlib import Path from unittest import TestCase from unittest.mock import call, patch @@ -196,6 +197,32 @@ class TestSettings(TestCase, TestMixin): 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_setting_value(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains): + """Test that the upgrade mechanism for settings correctly uses the new value when it's not a function""" + # 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__ = [ + ('values/old value', 'values/new value', [(True, 1)]) + ] + settings.__version__ = 99 + mocked_value.side_effect = [98, 1] + 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('values/old value')] + assert mocked_setValue.call_count == 2, 'Settings().setValue() should have been called twice' + assert mocked_setValue.call_args_list == [call('values/new value', True), call('settings/version', 99)] + mocked_contains.assert_called_once_with('values/old value') + mocked_remove.assert_called_once_with('values/old 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') @@ -218,3 +245,50 @@ class TestSettings(TestCase, TestMixin): 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')] + + def test_can_upgrade(self): + """Test the Settings.can_upgrade() method""" + # GIVEN: A Settings object + local_settings = Settings() + + # WHEN: can_upgrade() is run + result = local_settings.can_upgrade() + + # THEN: The result should be True + assert result is True, 'The settings should be upgradeable' + + def test_convert_value_setting_none_str(self): + """Test the Settings._convert_value() method when a setting is None and the default value is a string""" + # GIVEN: A settings object + # WHEN: _convert_value() is run + result = Settings()._convert_value(None, 'string') + + # THEN: The result should be an empty string + assert result == '', 'The result should be an empty string' + + def test_convert_value_setting_none_list(self): + """Test the Settings._convert_value() method when a setting is None and the default value is a list""" + # GIVEN: A settings object + # WHEN: _convert_value() is run + result = Settings()._convert_value(None, [None]) + + # THEN: The result should be an empty list + assert result == [], 'The result should be an empty list' + + def test_convert_value_setting_json_Path(self): + """Test the Settings._convert_value() method when a setting is JSON and represents a Path object""" + # GIVEN: A settings object + # WHEN: _convert_value() is run + result = Settings()._convert_value('{"__Path__": ["openlp", "core"]}', None) + + # THEN: The result should be a Path object + assert isinstance(result, Path), 'The result should be a Path object' + + def test_convert_value_setting_bool_str(self): + """Test the Settings._convert_value() method when a setting is supposed to be a boolean""" + # GIVEN: A settings object + # WHEN: _convert_value() is run + result = Settings()._convert_value('false', True) + + # THEN: The result should be False + assert result is False, 'The result should be False'