Skip to content
34 changes: 31 additions & 3 deletions test/test_open.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import ctypes
import os
import stat
import unittest
Expand All @@ -12,6 +13,22 @@
FILENAME = Path('one')


def is_windows_admin():
"""Check if the script is running in a terminal with admin privileges on Windows"""
if os.name == 'nt':
try:
return ctypes.windll.shell32.IsUserAnAdmin()
except Exception:
return False


IS_WINDOWS_USER = os.name == 'nt' and not is_windows_admin()
skip_if_symlink_creation_forbidden = unittest.skipIf(
IS_WINDOWS_USER,
'This test requires admin privileges to create symlink files on Windows',
)


@helpers.temps(safer.open)
@tdir
class TestSafer(unittest.TestCase):
Expand Down Expand Up @@ -129,8 +146,14 @@ def test_file_perms(self, safer_open):
fp.write('hello')
assert FILENAME.read_text() == 'hello'
mode = os.stat(FILENAME).st_mode
assert mode in (0o100664, 0o100644), stat.filemode(mode)
new_mode = mode & 0o100770

if os.name == 'posix':
assert mode in (0o100664, 0o100644), stat.filemode(mode)
new_mode = mode & 0o100770
elif os.name == 'nt':
new_mode = mode
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The AI had some sort of Windows-y test for modes here, I didn't look at it, you might try it...?
  2. You need another else to catch the other cases, whatever they might be:
else:
    assert False, f'Do not understand os.name = {os.name}'
  1. Should be # Instead, just check if it's a regular file without permission bit specifics - capital I, apostrophe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For:

  1. I wanted a human opinion, first. 😅
  2. Spot on! I'll get to that.
  3. Thanks a bunch for that fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for the Windows-y mode, I'll have to go through the code in more detail and check docs, because a chunk of it was new to me. I'll give feedback in a couple of hours, though.

else:
assert False, f'Do not understand os.name = {os.name}'

os.chmod(FILENAME, new_mode)
with safer_open(FILENAME, 'w') as fp:
Expand Down Expand Up @@ -183,6 +206,7 @@ def test_mode_t(self, safer_open):
fp.write('hello')
assert FILENAME.read_text() == 'hello'

@skip_if_symlink_creation_forbidden
def test_symlink_file(self, safer_open):
with safer_open(FILENAME, 'w') as fp:
fp.write('hello')
Expand All @@ -194,6 +218,7 @@ def test_symlink_file(self, safer_open):
fp.write('overwritten')
assert FILENAME.read_text() == 'overwritten'

@skip_if_symlink_creation_forbidden
def test_symlink_directory(self, safer_open):
FILENAME = Path('sub/test.txt')
with safer_open(FILENAME, 'w', make_parents=True) as fp:
Expand Down Expand Up @@ -227,4 +252,7 @@ def test_tempfile_perms(self, safer_open):
perms.append(os.stat(filename).st_mode)

assert perms == [perms[0]] * len(perms)
assert perms[0] in (0o100644, 0o100664)
if os.name == 'nt':
assert perms[0] == 0o100666
else:
assert perms[0] in (0o100644, 0o100664)
12 changes: 6 additions & 6 deletions test/test_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ def test_wrapper_bug():
@tdir
def test_wrapper_bug2():
with pytest.raises(NotImplementedError) as e:
fp = open(FILENAME, 'w')
safer.writer(fp, close_on_exit=True, temp_file=True)
with open(FILENAME, 'w') as fp:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I'm pretty sure you've found the issue!


So this change needs two non-obvious tweaks.

I didn't accidentally forget to open fp in a context in these - I'm using the close_on_exit flag, which should automatically close fp.

In this first test, I think you have actually revealed an issue:

with safer.writer(fp, close_on_exit=True, temp_file=True) as writer:might not be closing fp if there's an exception. I need to fix that. (It doesn't get detected in the Linux tests because the file handle gets closed immediately it gets destructed.)

So I need to make this change, or verify the bug doesn't exist, and then the first piece of code needs to change to:

fp = open(FILENAME, 'w')
with safer.writer(fp, close_on_exit=True, temp_file=True):
    pass

Probably the second code block should stay the same...

I'll make that change and let you know!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Our bug hunt pays off. :)

safer.writer(fp, close_on_exit=True, temp_file=True)
assert e.value.args == (safer.BUG_MESSAGE,)


Expand All @@ -211,10 +211,10 @@ def test_wrapper_bug3():
fp.write('hello, world')
assert FILENAME.read_text() == 'hello, world' # OK!

fp = open(FILENAME, 'w')
with safer.writer(fp, close_on_exit=True, temp_file=True):
fp.write('hello, world')
assert FILENAME.read_text() == '' # Fails!
with open(FILENAME, 'w') as fp:
with safer.writer(fp, close_on_exit=True, temp_file=True):
fp.write('')
assert FILENAME.read_text() == ''
finally:
safer.BUG_MESSAGE = bug

Expand Down