From b3d6c5c939704ea697539e78baf8661f243a5d32 Mon Sep 17 00:00:00 2001 From: Jernej Urankar Date: Tue, 28 Mar 2017 16:23:39 +0200 Subject: [PATCH 1/5] [FIX] Permission error when saving settings https://sentry.io/biolab/orange3/issues/242461786/ --- Orange/widgets/settings.py | 21 ++++---- Orange/widgets/tests/test_settings_handler.py | 48 ++++++++++++++++++- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/Orange/widgets/settings.py b/Orange/widgets/settings.py index a977b06f0f4..6f3b4be30ba 100644 --- a/Orange/widgets/settings.py +++ b/Orange/widgets/settings.py @@ -32,6 +32,7 @@ import copy import itertools import os +import logging import pickle import time import warnings @@ -40,6 +41,8 @@ from Orange.misc.environ import widget_settings_dir from Orange.widgets.utils import vartype +log = logging.getLogger(__name__) + __all__ = ["Setting", "SettingsHandler", "ContextSetting", "ContextHandler", "DomainContextHandler", "PerfectDomainContextHandler", @@ -397,15 +400,17 @@ def write_defaults(self): should overload the latter.""" filename = self._get_settings_filename() os.makedirs(os.path.dirname(filename), exist_ok=True) - - settings_file = open(filename, "wb") try: - self.write_defaults_file(settings_file) - except (EOFError, IOError, pickle.PicklingError): - settings_file.close() - os.remove(filename) - else: - settings_file.close() + settings_file = open(filename, "wb") + try: + self.write_defaults_file(settings_file) + except (EOFError, IOError, pickle.PicklingError): + settings_file.close() + os.remove(filename) + else: + settings_file.close() + except PermissionError: + log.error("PermissionError when trying to write defaults.", exc_info=True) def write_defaults_file(self, settings_file): """Write defaults for this widget class to a file diff --git a/Orange/widgets/tests/test_settings_handler.py b/Orange/widgets/tests/test_settings_handler.py index 044edd24461..e47aa9728bd 100644 --- a/Orange/widgets/tests/test_settings_handler.py +++ b/Orange/widgets/tests/test_settings_handler.py @@ -1,9 +1,12 @@ +# pylint: disable=protected-access from contextlib import contextmanager import os import pickle from tempfile import mkstemp +from sys import version_info +from platform import system import unittest -from unittest.mock import patch, Mock +from unittest.mock import patch, Mock, MagicMock import warnings from Orange.widgets.settings import SettingsHandler, Setting, SettingProvider,\ VERSION_KEY, rename_setting, Context, migrate_str_to_variable @@ -64,6 +67,49 @@ def test_write_defaults(self): os.remove(settings_file) + def test_write_PermissionError(self): + """ + Error may occur while trying to open file. Therefore file + cannot be closed. Tests if error is handled. + GH-2147 + """ + handler = SettingsHandler() + handler._get_settings_filename = lambda: "" + patch_target = "Orange.widgets.settings.open" if version_info >= (3, 5) \ + else "builtins.open" + patch_target_2 = "Orange.widgets.settings.SettingsHandler.write_defaults_file" + patch_target_3 = "os.makedirs" + + mock2 = MagicMock() + mock = MagicMock(side_effect=PermissionError) + with patch(patch_target, mock),\ + patch(patch_target_2, mock2),\ + patch(patch_target_3, mock2): + handler.write_defaults() + + def test_write_EOF_IO_Pickling_Errors(self): + """ + Tests if errors are handled. + GH-2147 + """ + patch_target_1 = "Orange.widgets.settings.open" if version_info >= (3, 5) \ + else "builtins.open" + patch_target_2 = "os.remove" + patch_target_3 = "os.makedirs" + patch_target_4 = "Orange.widgets.settings.SettingsHandler.write_defaults_file" + handler = SettingsHandler() + handler._get_settings_filename = lambda: "" + + mock = MagicMock() + with patch(patch_target_1, mock), patch(patch_target_2, mock), \ + patch(patch_target_3, mock): + with patch(patch_target_4, side_effect=EOFError): + handler.write_defaults() + with patch(patch_target_4, side_effect=IOError): + handler.write_defaults() + with patch(patch_target_4, side_effect=pickle.PicklingError): + handler.write_defaults() + def test_initialize_widget(self): handler = SettingsHandler() handler.defaults = {'default': 42, 'setting': 1} From 42387e36e92b6aa1ecc12639cb4b14156a42df50 Mon Sep 17 00:00:00 2001 From: astaric Date: Thu, 20 Apr 2017 14:27:30 +0200 Subject: [PATCH 2/5] Refactor tests. --- Orange/widgets/settings.py | 9 ++- Orange/widgets/tests/test_settings_handler.py | 69 ++++++++----------- 2 files changed, 33 insertions(+), 45 deletions(-) diff --git a/Orange/widgets/settings.py b/Orange/widgets/settings.py index 6f3b4be30ba..62f20a13841 100644 --- a/Orange/widgets/settings.py +++ b/Orange/widgets/settings.py @@ -404,13 +404,16 @@ def write_defaults(self): settings_file = open(filename, "wb") try: self.write_defaults_file(settings_file) - except (EOFError, IOError, pickle.PicklingError): + except (EOFError, IOError, pickle.PicklingError) as ex: + log.error("Could not write default settings for %s (%s).", + self.widget_class, type(ex).__name__) settings_file.close() os.remove(filename) else: settings_file.close() - except PermissionError: - log.error("PermissionError when trying to write defaults.", exc_info=True) + except PermissionError as ex: + log.error("Could not write default settings for %s (%s).", + self.widget_class, type(ex).__name__) def write_defaults_file(self, settings_file): """Write defaults for this widget class to a file diff --git a/Orange/widgets/tests/test_settings_handler.py b/Orange/widgets/tests/test_settings_handler.py index e47aa9728bd..92a72c1b756 100644 --- a/Orange/widgets/tests/test_settings_handler.py +++ b/Orange/widgets/tests/test_settings_handler.py @@ -2,12 +2,13 @@ from contextlib import contextmanager import os import pickle -from tempfile import mkstemp -from sys import version_info -from platform import system +from tempfile import mkstemp, NamedTemporaryFile + import unittest -from unittest.mock import patch, Mock, MagicMock +from unittest.mock import patch, Mock import warnings + +from Orange.tests import named_file from Orange.widgets.settings import SettingsHandler, Setting, SettingProvider,\ VERSION_KEY, rename_setting, Context, migrate_str_to_variable @@ -67,49 +68,33 @@ def test_write_defaults(self): os.remove(settings_file) - def test_write_PermissionError(self): - """ - Error may occur while trying to open file. Therefore file - cannot be closed. Tests if error is handled. - GH-2147 - """ - handler = SettingsHandler() - handler._get_settings_filename = lambda: "" - patch_target = "Orange.widgets.settings.open" if version_info >= (3, 5) \ - else "builtins.open" - patch_target_2 = "Orange.widgets.settings.SettingsHandler.write_defaults_file" - patch_target_3 = "os.makedirs" - - mock2 = MagicMock() - mock = MagicMock(side_effect=PermissionError) - with patch(patch_target, mock),\ - patch(patch_target_2, mock2),\ - patch(patch_target_3, mock2): - handler.write_defaults() - - def test_write_EOF_IO_Pickling_Errors(self): - """ - Tests if errors are handled. - GH-2147 - """ - patch_target_1 = "Orange.widgets.settings.open" if version_info >= (3, 5) \ - else "builtins.open" - patch_target_2 = "os.remove" - patch_target_3 = "os.makedirs" - patch_target_4 = "Orange.widgets.settings.SettingsHandler.write_defaults_file" + def test_write_defaults_handles_permission_error(self): handler = SettingsHandler() - handler._get_settings_filename = lambda: "" - mock = MagicMock() - with patch(patch_target_1, mock), patch(patch_target_2, mock), \ - patch(patch_target_3, mock): - with patch(patch_target_4, side_effect=EOFError): - handler.write_defaults() - with patch(patch_target_4, side_effect=IOError): + with named_file("") as f: + handler._get_settings_filename = lambda: f + + with patch('Orange.widgets.settings.open', create=True) as mocked_open: + mocked_open.side_effect = PermissionError() + handler.write_defaults() - with patch(patch_target_4, side_effect=pickle.PicklingError): + + def test_write_defaults_handles_writing_errors(self): + handler = SettingsHandler() + + for error in (EOFError, IOError, pickle.PicklingError): + f = NamedTemporaryFile("wt", delete=False) + f.close() # so it can be opened on windows + handler._get_settings_filename = lambda x=f: x.name + + with patch.object(handler, "write_defaults_file") as mocked_write: + mocked_write.side_effect = error() + handler.write_defaults() + # Corrupt setting files should be removed + self.assertFalse(os.path.exists(f.name)) + def test_initialize_widget(self): handler = SettingsHandler() handler.defaults = {'default': 42, 'setting': 1} From 613180a855d6aa9d2fe52f487c16b8322175cd05 Mon Sep 17 00:00:00 2001 From: astaric Date: Thu, 20 Apr 2017 14:28:35 +0200 Subject: [PATCH 3/5] tests: Discover tests in Orange.widgets.tests package --- Orange/widgets/tests/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Orange/widgets/tests/__init__.py b/Orange/widgets/tests/__init__.py index 00008d11efe..c1f457e795d 100644 --- a/Orange/widgets/tests/__init__.py +++ b/Orange/widgets/tests/__init__.py @@ -10,6 +10,7 @@ def load_tests(loader, tests, pattern): return unittest.TestSuite([]) widgets_dir = os.path.dirname(Orange.widgets.__file__) + widget_tests_dir = os.path.dirname(__file__) top_level_dir = os.path.dirname(os.path.dirname(Orange.__file__)) if loader is None: @@ -20,6 +21,9 @@ def load_tests(loader, tests, pattern): load_tests._in_load_tests = True try: all_tests = [ + # Widgets in this package are discovered separately to avoid + # infinite recursion (see the guard above) + loader.discover(widget_tests_dir, pattern, widget_tests_dir), loader.discover(widgets_dir, pattern, top_level_dir) ] finally: From 7aa4d4e97a160049e979d45ba7ff58b269e18383 Mon Sep 17 00:00:00 2001 From: astaric Date: Thu, 20 Apr 2017 14:29:04 +0200 Subject: [PATCH 4/5] Skip a stalling test --- Orange/widgets/tests/test_highcharts.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Orange/widgets/tests/test_highcharts.py b/Orange/widgets/tests/test_highcharts.py index 2dbc9bddf2e..63cfe050e35 100644 --- a/Orange/widgets/tests/test_highcharts.py +++ b/Orange/widgets/tests/test_highcharts.py @@ -1,6 +1,4 @@ import time -import os -import sys import unittest from AnyQt.QtCore import Qt, QPoint, QObject @@ -35,9 +33,7 @@ def test_svg_is_svg(self): self.assertEqual(svg[:5], '') - @unittest.skipIf(os.environ.get('APPVEYOR'), 'test stalls on AppVeyor') - @unittest.skipIf(sys.version_info[:2] <= (3, 4), - 'the second iteration stalls on Travis / Py3.4') + @unittest.skip("Does not work") def test_selection(self): class NoopBridge(QObject): From 8f603c3f013da66b7892ff2c2239f19179c552a4 Mon Sep 17 00:00:00 2001 From: astaric Date: Thu, 20 Apr 2017 14:53:42 +0200 Subject: [PATCH 5/5] Fix failing setting_handler tests on py3.4 --- Orange/widgets/tests/test_settings_handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Orange/widgets/tests/test_settings_handler.py b/Orange/widgets/tests/test_settings_handler.py index 92a72c1b756..7975fa38bb4 100644 --- a/Orange/widgets/tests/test_settings_handler.py +++ b/Orange/widgets/tests/test_settings_handler.py @@ -317,9 +317,9 @@ def override_default_settings(self, widget, defaults=None): h = SettingsHandler() h.widget_class = widget + h.defaults = defaults filename = h._get_settings_filename() - with open(filename, "wb") as f: - pickle.dump(defaults, f) + h.write_defaults() yield