[FIX] Permission error when saving settings #2147
Conversation
Codecov Report
@@ 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 |
There was a problem hiding this comment.
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.
Orange/widgets/settings.py
Outdated
| else: | ||
| settings_file.close() | ||
| except PermissionError: # pragma: no cover | ||
| pass |
There was a problem hiding this comment.
Is there any reason for not having one try with two except statements here?
There was a problem hiding this comment.
@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.
| """ | ||
| _, settings_file = mkstemp(suffix='.ini') | ||
| handler = SettingsHandler() | ||
| handler._get_settings_filename = lambda: settings_file # pylint: disable=W0212 |
There was a problem hiding this comment.
Please use verbose warning names when disabling them.
| _, settings_file = mkstemp(suffix='.ini') | ||
| handler = SettingsHandler() | ||
| handler._get_settings_filename = lambda: settings_file # pylint: disable=W0212 | ||
| from sys import version_info |
There was a problem hiding this comment.
Imports go to the top of the file.
| cannot be closed. Tests if error is handled. | ||
| GH-2147 | ||
| """ | ||
| _, settings_file = mkstemp(suffix='.ini') |
| with patch(patch_target, side_effect=IOError): | ||
| handler.write_defaults() | ||
| with patch(patch_target, side_effect=pickle.PicklingError): | ||
| handler.write_defaults() |
There was a problem hiding this comment.
You could also check that the file gets deleted (as it contains invalid settings).
Orange/widgets/settings.py
Outdated
| else: | ||
| settings_file.close() | ||
| except PermissionError: | ||
| pass |
There was a problem hiding this comment.
It might be a good idea to log the problem as it looks like this will happen every time.
|
Some tests fail on windows: |
There was a problem hiding this comment.
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.
| else "builtins.open" | ||
| with patch(patch_target, side_effect=PermissionError): | ||
| handler.write_defaults() | ||
| try: |
There was a problem hiding this comment.
This is not a way to solve the problem. If remove sometimes fails, figure out why and solve the cause.
| handler.write_defaults() | ||
| with patch(patch_target, side_effect=pickle.PicklingError): | ||
| handler.write_defaults() | ||
| if system() != "Windows": |
There was a problem hiding this comment.
Same here, figure out why the files does not get deleted on windows and then solve the cause of the problem.
jerneju
changed the title
jerneju
changed the title
|
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. |
Issue
Sometimes settings cannot be saved due to file permission issues.
https://sentry.io/biolab/orange3/issues/242461786/
Description of changes
try block
Includes