-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for recursive symlink issue #4669 #4670
Conversation
… the recusrive symlink error and moves on. It is possibble that the try/except belongs in the utils but I wanted to limit the scope.
self.assertTrue('untitled.txt' in contents) | ||
self.assertEqual(contents['untitled.txt'], file_model) | ||
# recusrive symlinks should not be shown in the contents manager | ||
self.assertFalse('recusrive' in contents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling of 'recursive'.
Also, we should use the assertIn
and assertNotIn
methods rather than assertTrue and assertFalse. The more specific methods give more useful messages when things fail.
self.assertIn('untitled.txt', contents) | ||
self.assertEqual(contents['untitled.txt'], file_model) | ||
# recursive symlinks should not be shown in the contents manager | ||
self.assertNotIn('recursive', contents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the spelling, but this test is now failing on Windows:
======================================================================
FAIL: test_recursive_symlink (notebook.services.contents.tests.test_manager.TestFileContentsManager)
----------------------------------------------------------------------
Traceback (most recent call last):
File "C:\Miniconda36-x64\lib\site-packages\ipython_genutils\testing\decorators.py", line 186, in skipper_func
return f(*args, **kwargs)
File "C:\projects\notebook\notebook\services\contents\tests\test_manager.py", line 150, in test_recursive_symlink
self.assertNotIn('recursive', contents)
AssertionError: 'recursive' unexpectedly found in {'recursive': {'name': 'recursive', 'path': 'test recursive symlink/recursive', 'last_modified': datetime.datetime(2019, 6, 25, 8, 35, 46, 553212, tzinfo=<notebook._tz.tzUTC object at 0x000000B0AB495D68>), 'created': datetime.datetime(2019, 6, 25, 8, 35, 46, 553212, tzinfo=<notebook._tz.tzUTC object at 0x000000B0AB495D68>), 'content': None, 'format': None, 'mimetype': None, 'size': 0, 'writable': True, 'type': 'file'}, 'untitled.txt': {'name': 'untitled.txt', 'path': 'test recursive symlink/untitled.txt', 'last_modified': datetime.datetime(2019, 6, 25, 8, 35, 46, 553212, tzinfo=<notebook._tz.tzUTC object at 0x000000B0AB495D68>), 'created': datetime.datetime(2019, 6, 25, 8, 35, 46, 553212, tzinfo=<notebook._tz.tzUTC object at 0x000000B0AB495D68>), 'content': None, 'format': None, 'mimetype': 'text/plain', 'size': 0, 'writable': True, 'type': 'file'}}
I guess Windows doesn't handle this in the same way Unix does. It's probably fine to just skip the test on Windows completely. I think it's unusual to use symlinks on Windows anyway.
I've got the tests passing. I was informed of a related issue someone at EuXFEL encountered, where there was a permissions error stat-ing a symlink. So I've broadened this to catch all OSError, logging the exception details for anything besides the recursive symlink case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This is a potential fix to issue #4669. The fix simply catches the
recursive symlink error and moves on. It is possible that the
try/except belongs in the utils but I wanted to limit the scope.