[FIX] Permission error when saving settings by jerneju · Pull Request #2147 · biolab/orange3

jerneju · 2017-03-28T14:25:29Z

Issue

Sometimes settings cannot be saved due to file permission issues.
https://sentry.io/biolab/orange3/issues/242461786/

Description of changes

try block

Includes

Code changes
Tests
Documentation

jerneju · 2017-03-28T14:39:01Z

https://sentry.io/biolab/orange3/issues/242461786/

codecov-io · 2017-03-28T14:40:11Z

Codecov Report

Merging #2147 into master will increase coverage by 4.52%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2147      +/-   ##
=========================================
+ Coverage   67.78%   72.3%   +4.52%     
=========================================
  Files         319     319              
  Lines       54940   54962      +22     
=========================================
+ Hits        37240   39740    +2500     
+ Misses      17700   15222    -2478

janezd

Besides addressing the other comments, add tests. Patch open to raise the permission error, and write_default_file to raise other three errors. Also patch the function that you will use for showing the dialog with a mock, and check whether the mock is called.

If somebody later changes this function to do things differently, the test may fail because of being too specific. There's nothing wrong with that, as it will force her/him to rewrite the tests.

janezd · 2017-03-31T21:04:52Z

+            else:
+                settings_file.close()
+        except PermissionError: # pragma: no cover
+            pass


Is there any reason for not having one try with two except statements here?

@astaric, I suppose that we should warn the user that settings were not saved, using a critical warning dialog? We'd do it just once per session.

This also goes for the exceptions caught above.

astaric · 2017-04-06T08:20:28Z

+        """
+        _, settings_file = mkstemp(suffix='.ini')
+        handler = SettingsHandler()
+        handler._get_settings_filename = lambda: settings_file  # pylint: disable=W0212


Please use verbose warning names when disabling them.

astaric · 2017-04-06T08:22:31Z

+        _, settings_file = mkstemp(suffix='.ini')
+        handler = SettingsHandler()
+        handler._get_settings_filename = lambda: settings_file  # pylint: disable=W0212
+        from sys import version_info


Imports go to the top of the file.

astaric · 2017-04-06T08:59:30Z

+        cannot be closed. Tests if error is handled.
+        GH-2147
+        """
+        _, settings_file = mkstemp(suffix='.ini')


where is this file deleted?

astaric · 2017-04-06T09:00:12Z

+        with patch(patch_target, side_effect=IOError):
+            handler.write_defaults()
+        with patch(patch_target, side_effect=pickle.PicklingError):
+            handler.write_defaults()


You could also check that the file gets deleted (as it contains invalid settings).

astaric · 2017-04-06T09:01:48Z

+            else:
+                settings_file.close()
+        except PermissionError:
+            pass


It might be a good idea to log the problem as it looks like this will happen every time.

Done

astaric · 2017-04-07T07:56:04Z

Some tests fail on windows:

======================================================================
ERROR: test_write_PermissionError (Orange.widgets.tests.test_settings_handler.SettingHandlerTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\appveyor\testenv\lib\site-packages\Orange\widgets\tests\test_settings_handler.py", line 82, in test_write_PermissionError
    os.remove(settings_file)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tmpi03y3agg.ini'
======================================================================
FAIL: test_write_EOF_IO_Pickling_Errors (Orange.widgets.tests.test_settings_handler.SettingHandlerTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\appveyor\testenv\lib\site-packages\Orange\widgets\tests\test_settings_handler.py", line 99, in test_write_EOF_IO_Pickling_Errors
    self.assertFalse(os.path.exists(settings_file))
AssertionError: True is not false
----------------------------------------------------------------------
Ran 634 tests in 69.664s
FAILED (failures=1, errors=1, skipped=5)

astaric

Failing tests indicate that something weird is going on. Try not to "fix" tests with try: except, as that defeats the purpose of the test.

If you take a look at the error message given by a failing test, it states:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process ...

This indicates that a file could not be deleted as it is still open. Figure out why (who opened it) and properly close it. Should fix both of the problems.

astaric · 2017-04-12T11:37:40Z

+            else "builtins.open"
+        with patch(patch_target, side_effect=PermissionError):
+            handler.write_defaults()
+        try:


This is not a way to solve the problem. If remove sometimes fails, figure out why and solve the cause.

astaric · 2017-04-12T11:38:14Z

+            handler.write_defaults()
+        with patch(patch_target, side_effect=pickle.PicklingError):
+            handler.write_defaults()
+            if system() != "Windows":


Same here, figure out why the files does not get deleted on windows and then solve the cause of the problem.

https://sentry.io/biolab/orange3/issues/242461786/

astaric · 2017-04-20T12:38:51Z

I have simplified the tests a bit and hopefully fixed the problem that caused these tests not to be run on python3.5 on travis.

janezd previously requested changes Mar 31, 2017

View reviewed changes

astaric reviewed Apr 6, 2017

View reviewed changes

astaric self-assigned this Apr 7, 2017

astaric added this to the 3.4.2 milestone Apr 7, 2017

astaric reviewed Apr 12, 2017

View reviewed changes

jerneju changed the title ~~[FIX] Permission error when saving settings~~ [WIP][FIX] Permission error when saving settings Apr 14, 2017

astaric modified the milestones: 3.4.2, future Apr 19, 2017

[FIX] Permission error when saving settings

b3d6c5c

https://sentry.io/biolab/orange3/issues/242461786/

jerneju changed the title ~~[WIP][FIX] Permission error when saving settings~~ [FIX] Permission error when saving settings Apr 20, 2017

astaric added 4 commits April 20, 2017 14:49

Refactor tests.

42387e3

tests: Discover tests in Orange.widgets.tests package

613180a

Skip a stalling test

7aa4d4e

Fix failing setting_handler tests on py3.4

8f603c3

astaric merged commit 29835e2 into biolab:master Apr 20, 2017

jerneju deleted the file-save-permission branch April 20, 2017 13:49

Uh oh!

Conversation

jerneju commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description of changes

Includes

Uh oh!

jerneju commented Mar 28, 2017

Uh oh!

codecov-io commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

janezd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerneju commented Mar 28, 2017 •

edited

Loading

codecov-io commented Mar 28, 2017 •

edited

Loading

astaric commented Apr 7, 2017 •

edited

Loading