Unit tests and some fixes for improved plugin settings processing

src/octoprint/plugin/types.py

@@ -1209,7 +1209,7 @@ class SettingsPlugin(OctoPrintPlugin):
 
 		:return: the current settings of the plugin, as a dictionary
 		"""
-		data = self._settings.get([], asdict=True, merged=True)
+		data = self._settings.get_all_data()
 		if self.config_version_key in data:
 			del data[self.config_version_key]
 		return data
@@ -1240,6 +1240,8 @@ class SettingsPlugin(OctoPrintPlugin):
 
 		# get the current data
 		current = self._settings.get_all_data()
+		if current is None:
+			current = dict()
 
 		# merge our new data on top of it
 		new_current = octoprint.util.dict_merge(current, data)
@@ -1255,7 +1257,11 @@ class SettingsPlugin(OctoPrintPlugin):
 		to_persist = dict(diff)
 		if version:
 			to_persist[self.config_version_key] = version
-		self._settings.set([], to_persist)
+
+		if to_persist:
+			self._settings.set([], to_persist)
+		else:
+			self._settings.clean_all_data()
 
 		return diff
 
@@ -1383,9 +1389,6 @@ class SettingsPlugin(OctoPrintPlugin):
 			# diff => persist only that
 			self._settings.set([], diff)
 
-		# finally save everything
-		self._settings.save()
-
 	def on_settings_initialized(self):
 		"""
 		Called after the settings have been initialized and - if necessary - also been migrated through a call to

src/octoprint/server/__init__.py

@@ -236,9 +236,10 @@ class Server(object):
 				if stored_version is None or stored_version < settings_version:
 					settings_migrator(settings_version, stored_version)
 					implementation._settings.set_int([octoprint.plugin.SettingsPlugin.config_version_key], settings_version)
-					implementation._settings.save()
 
 			implementation.on_settings_cleanup()
+			implementation._settings.save()
+
 			implementation.on_settings_initialized()
 
 		pluginManager.implementation_inject_factories=[octoprint_plugin_inject_factory, settings_plugin_inject_factory]

tests/plugin/test_types_settings.py

@@ -0,0 +1,313 @@
+import unittest
+import mock
+
+import octoprint.plugin
+
+class TestSettingsPlugin(unittest.TestCase):
+
+	def setUp(self):
+		self.settings = mock.MagicMock()
+
+		self.plugin = octoprint.plugin.SettingsPlugin()
+		self.plugin._settings = self.settings
+
+	def test_on_settings_cleanup(self):
+		"""Tests that after cleanup only minimal config is left in storage."""
+
+		### setup
+
+		# settings defaults
+		defaults = dict(
+			foo=dict(
+				a=1,
+				b=2,
+				l1=["some", "list"],
+				l2=["another", "list"]
+			),
+			bar=True,
+			fnord=None
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		# stored config, containing one redundant entry (bar=True, same as default)
+		in_config = dict(
+			foo=dict(
+				l1=["some", "other", "list"],
+				l2=["another", "list"],
+				l3=["a", "third", "list"]
+			),
+			bar=True,
+			fnord=dict(
+				c=3,
+				d=4
+			)
+		)
+		self.settings.get_all_data.return_value = in_config
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# minimal config (current without redundant value) should have been set
+		expected = dict(
+			foo=dict(
+				l1=["some", "other", "list"],
+				l3=["a", "third", "list"]
+			),
+			fnord=dict(
+				c=3,
+				d=4
+			)
+		)
+		self.settings.set.assert_called_once_with([], expected)
+
+	def test_on_settings_cleanup_configversion(self):
+		"""Tests that set config version is always left stored."""
+
+		### setup
+
+		defaults = dict(
+			foo="fnord"
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		in_config = dict(
+			_config_version=1,
+			foo="fnord"
+		)
+		self.settings.get_all_data.return_value = in_config
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# minimal config incl. config version should have been set
+		self.settings.set.assert_called_once_with([], dict(_config_version=1))
+
+	def test_on_settings_cleanup_noconfigversion(self):
+		"""Tests that config versions of None are cleaned from stored data."""
+
+		### setup
+
+		defaults = dict(
+			foo="bar"
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		# stored config version is None
+		in_config = dict(
+			_config_version=None,
+			foo="fnord"
+		)
+		self.settings.get_all_data.return_value = in_config
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# minimal config without config version should have been set
+		self.settings.set.assert_called_once_with([], dict(foo="fnord"))
+
+	def test_on_settings_cleanup_emptydiff(self):
+		"""Tests that settings are cleaned up if the diff data <-> defaults is empty."""
+
+		### setup
+
+		defaults = dict(
+			foo="bar"
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		# current stored config, same as defaults
+		in_config = dict(
+			foo="bar"
+		)
+		self.settings.get_all_data.return_value = in_config
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# should have been cleared
+		self.settings.clean_all_data.assert_called_once_with()
+
+	def test_on_settings_cleanup_nosuchpath(self):
+		"""Tests that no processing is done if nothing is stored in settings."""
+
+		from octoprint.settings import NoSuchSettingsPath
+
+		### setup
+
+		# simulate no settings stored in config.yaml
+		self.settings.get_all_data.side_effect = NoSuchSettingsPath()
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# only get_all_data should have been called
+		self.settings.get_all_data.assert_called_once_with(merged=False, incl_defaults=False, error_on_path=True)
+		self.assertTrue(len(self.settings.method_calls) == 1)
+
+	def test_on_settings_cleanup_none(self):
+		"""Tests the None entries in config get cleaned up."""
+
+		### setup
+
+		# simulate None entry in config.yaml
+		self.settings.get_all_data.return_value = None
+
+		### execute
+
+		self.plugin.on_settings_cleanup()
+
+		### assert
+
+		# should have been cleaned
+		self.settings.clean_all_data.assert_called_once_with()
+
+	def test_on_settings_save(self):
+		"""Tests that only the diff is saved."""
+
+		### setup
+
+		current = dict(
+			foo="bar"
+		)
+		self.settings.get_all_data.return_value = current
+
+		defaults = dict(
+			foo="foo",
+			bar=dict(
+				a=1,
+				b=2
+			)
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		### execute
+
+		data = dict(
+			foo="fnord",
+			bar=dict(
+				a=1,
+				b=2
+			)
+		)
+		diff = self.plugin.on_settings_save(data)
+
+		### assert
+
+		# the minimal diff should have been saved
+		expected = dict(
+			foo="fnord"
+		)
+		self.settings.set.assert_called_once_with([], expected)
+
+		self.assertEquals(diff, expected)
+
+	def test_on_settings_save_nodiff(self):
+		"""Tests that data is cleaned if there's not difference between data and defaults."""
+
+		### setup
+
+		self.settings.get_all_data.return_value = None
+
+		defaults = dict(
+			foo="bar",
+			bar=dict(
+				a=1,
+				b=2,
+				l=["some", "list"]
+			)
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		### execute
+
+		data = dict(foo="bar")
+		diff = self.plugin.on_settings_save(data)
+
+		### assert
+
+		self.settings.clean_all_data.assert_called_once_with()
+		self.assertEquals(diff, dict())
+
+	def test_on_settings_save_configversion(self):
+		"""Tests that saved data gets stripped config version and set correct one."""
+
+		### setup
+
+		self.settings.get_all_data.return_value = None
+
+		defaults = dict(
+			foo="bar"
+		)
+		self.plugin.get_settings_defaults = mock.MagicMock()
+		self.plugin.get_settings_defaults.return_value = defaults
+
+		version = 1
+		self.plugin.get_settings_version = mock.MagicMock()
+		self.plugin.get_settings_version.return_value = version
+
+		### execute
+
+		data = dict(_config_version=None, foo="bar")
+		diff = self.plugin.on_settings_save(data)
+
+		### assert
+
+		expected_diff = dict()
+		expected_set = dict(_config_version=version)
+
+		# while there was no diff, we should still have saved the new config version
+		self.settings.set.assert_called_once_with([], expected_set)
+
+		self.assertEquals(diff, expected_diff)
+
+	def test_on_settings_load(self):
+		"""Tests that on_settings_load returns what's stored in the config, without config version."""
+
+		### setup
+
+		# current data incl. config version
+		current = dict(
+			_config_version=3,
+			foo="bar",
+			fnord=dict(
+				a=1,
+				b=2,
+				l=["some", "list"]
+			)
+		)
+
+		# expected is current without _config_version - we make the copy now
+		# since our current dict will be modified by the test
+		expected = dict(current)
+		del expected["_config_version"]
+
+		self.settings.get_all_data.return_value = expected
+
+		### execute
+
+		result = self.plugin.on_settings_load()
+
+		### assert
+
+		self.assertEquals(result, expected)