Fix Failing Tests on Windows (WIP) #27

Chiemezuo · 2024-10-22T13:16:11Z

This is in line with #8

I'm working on getting the test cases to pass for Windows.

Upon installation, 6 of the 70 existing tests were failing. Here's a short summary of them:

=================================================== short test summary info =================================================== 
FAILED test/test_open.py::TestSafer::test_file_perms - AssertionError: -rw-rw-rw-
FAILED test/test_open.py::TestSafer::test_symlink_directory - OSError: [WinError 1314] A required privilege is not held by the client: 'sub' -> 'sub.sym'
FAILED test/test_open.py::TestSafer::test_symlink_file - OSError: [WinError 1314] A required privilege is not held by the client: 'one' -> 'one.sym'
FAILED test/test_open.py::TestSafer::test_tempfile_perms - assert 33206 in (33188, 33204)
FAILED test/test_writer.py::test_wrapper_bug2 - PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\...
FAILED test/test_writer.py::test_wrapper_bug3 - PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\...

Fixed tests

Summary by Sourcery

Bug Fixes:

Fix the test_file_perms test to handle file permission checks correctly on Windows by adjusting the logic to account for Windows-specific file handling.

sourcery-ai · 2024-10-22T13:16:14Z

Reviewer's Guide by Sourcery

This pull request addresses failing tests on Windows platforms, focusing on file permission and symlink-related issues. The changes primarily involve modifying the test suite to account for differences in file handling between UNIX and Windows systems.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Modify file permission checks in test cases to accommodate Windows	Add conditional logic to handle file permission checks differently on Windows Replace specific permission bit checks with a more general file type check on Windows Retain original UNIX-specific permission checks for POSIX systems	`test/test_open.py`
Address symlink-related test failures on Windows	Identify symlink creation issues due to privilege restrictions on Windows Mark symlink-related tests as failing and needing further investigation	`test/test_open.py`
Investigate file access issues in wrapper-related tests	Identify PermissionError occurrences in wrapper bug tests Mark wrapper-related tests as failing and needing further investigation	`test/test_writer.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @Chiemezuo - I've reviewed your changes - here's some feedback:

Overall Comments:

While we appreciate the effort to fix Windows compatibility, bypassing permission checks on Windows is not the ideal solution. Consider finding Windows-equivalent ways to perform these security checks rather than removing them.
This PR only addresses one of the six failing tests. Please continue working on the other failures, particularly the symlink tests and file access issues, to ensure full Windows compatibility.
For cross-platform development, look into Windows-specific APIs or libraries that can provide functionality equivalent to what's being tested on Unix systems. This will lead to a more robust and secure solution across all platforms.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 2 issues found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2024-10-22T13:17:46Z

test/test_open.py

+        # UNIX systems view and manipulate file permissions as bits
+        if os.name is 'posix':
+            assert mode in (0o100664, 0o100644), stat.filemode(mode)
+            new_mode = mode & 0o100770
+        # disparity in Windows file handling
+        elif os.name is 'nt':
+            # instead, just check if its a regular file without permission bit specifics
+            new_mode = mode


suggestion (testing): Consider adding specific Windows permission checks

While the current change allows the test to pass on Windows, it doesn't verify any specific permission attributes. Consider adding Windows-specific checks to ensure the file has appropriate permissions, such as checking if it's readable and writable.

if os.name == 'posix': assert mode in (0o100664, 0o100644), stat.filemode(mode) new_mode = mode & 0o100770 elif os.name == 'nt': import win32security import ntsecuritycon as con sd = win32security.GetFileSecurity(filename, win32security.DACL_SECURITY_INFORMATION) dacl = sd.GetSecurityDescriptorDacl() assert dacl.GetEffectiveRightsFromAcl({win32security.ConvertSidToStringSid(win32security.GetTokenInformation(win32security.OpenProcessToken(win32api.GetCurrentProcess(), win32security.TOKEN_QUERY), win32security.TokenUser)[0]): con.FILE_GENERIC_READ | con.FILE_GENERIC_WRITE}) new_mode = mode

sourcery-ai · 2024-10-22T13:17:46Z

test/test_open.py

+        if os.name is 'posix':
+            assert mode in (0o100664, 0o100644), stat.filemode(mode)
+            new_mode = mode & 0o100770
+        # disparity in Windows file handling
+        elif os.name is 'nt':
+            # instead, just check if its a regular file without permission bit specifics
+            new_mode = mode


suggestion (testing): Consider using unittest.skipIf for OS-specific tests

Instead of using if-else statements in the test, consider using unittest.skipIf decorators to clearly separate OS-specific test cases. This would improve test readability and maintenance.

@unittest.skipIf(os.name != 'posix', "Test specific to POSIX systems") def test_posix_file_mode(self): assert mode in (0o100664, 0o100644), stat.filemode(mode) new_mode = mode & 0o100770 @unittest.skipIf(os.name != 'nt', "Test specific to Windows systems") def test_windows_file_mode(self): new_mode = mode

Chiemezuo · 2024-10-22T17:39:54Z

@rec
The symlink directory and files tests will work only on the condition that the tests are being run in a terminal that contains admin privileges. Unfortunately, admin privileges are not given to terminals by default (but can be changed). I set the test to skip for Windows if admin privileges are not given (instead of failing), but they will pass if given (or will fail if the source of errors is not from privileges).

rec

Good stuff, only tiny quibbles here!

I'll take a look later today at the last two tests, I might be able to just guess.

rec · 2024-10-24T09:38:34Z

test/test_open.py

+    if os.name == 'nt':
+        try:
+            return ctypes.windll.shell32.IsUserAnAdmin()
+        except:


Bare except is a bad idea because you catch things that aren't actually Exception - things like OutOfMemoryError or KeyboardInterrupt: https://stackoverflow.com/questions/54948548/what-is-wrong-with-using-a-bare-except

I think the CI (that I just enabled) will catch it, but if not, here's my general purpose lint that I use: https://github.com/rec/dotfiles/blob/master/bin/run-test

Thanks for the review!
I'll get started on implementing your recommendations.

Would except Exception as e work just fine for this case?

Yes, that would work much better!

Linting would reveal a tiny defect: you don't use the new variable e.

Best is exception Exception:.

On linting

The few years have seen a significant change in how people write Python. More and more, projects simply require you to run ruff with all the features turned on, on your code.

I think this is great - the code gets standardized, common errors and defects are avoided, and ruff is blazingly fast.

You should get into the habit of using ruff with a lot of stuff turned on, and also mypy, even on trivial code, because it catches so many bugs.

rec · 2024-10-24T09:41:55Z

test/test_open.py

+        # disparity in Windows file handling
+        elif os.name == 'nt':
+            # instead, just check if its a regular file without permission bit specifics
+            new_mode = mode


The AI had some sort of Windows-y test for modes here, I didn't look at it, you might try it...?

You need another else to catch the other cases, whatever they might be:

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

Should be # Instead, just check if it's a regular file without permission bit specifics - capital I, apostrophe.

For:

I wanted a human opinion, first. 😅

Spot on! I'll get to that.

Thanks a bunch for that fix.

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.

rec · 2024-10-24T09:43:31Z

test/test_open.py

@@ -184,6 +202,12 @@ def test_mode_t(self, safer_open):
        assert FILENAME.read_text() == 'hello'

    def test_symlink_file(self, safer_open):
+        # check if we are on Windows and have admin rights
+        if os.name == 'nt' and not is_windows_admin():


You should instead decorate this test case with https://docs.python.org/3/library/unittest.html#unittest.skipIf

Thanks!
I've done this.

rec · 2024-10-24T09:43:46Z

test/test_open.py

@@ -196,6 +220,13 @@ def test_symlink_file(self, safer_open):

    def test_symlink_directory(self, safer_open):
        FILENAME = Path('sub/test.txt')
+


Here too...

Chiemezuo · 2024-10-25T07:14:01Z

Looks good for another round of reviews

PS: I'll look deeper into the Windows-y command.

rec · 2024-10-25T10:35:56Z

So it's very natural to demonstrate that you comprehend and are keeping up, but a magic secret to appearing to be a more senior programmer is only talking about operative or actionable questions.

Overall, you don't need to respond to comments if you agree with them, or say what you are going to do, unless there's a question! :-D (Also, you should use the review feature to group together comments in one email, not that it's a big deal, particularly with such a small project.)

Computer programming is a very noisy occupation so I spend extra care in only sending people messages which are actionable, i.e., ask them to do something, or ask a question, or give them information they asked for.

Otherwise, people simply miss stuff in the traffic.

This also leaves more time for actual chat. 🔈

As I said, it's extremely natural, so don't think it's some sort of sin. Mostly you discover this somewhat later, when you have multiple junior programmers each trying to out-volume the other in progress reports... 😈

Chiemezuo · 2024-10-31T13:31:45Z

@rec A pending workflow approval

rec · 2024-11-02T12:29:58Z

test/test_writer.py

@@ -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:


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!

Nice! Our bug hunt pays off. :)

rec · 2024-11-02T12:32:24Z

test/test_open.py

@@ -183,6 +203,11 @@ def test_mode_t(self, safer_open):
            fp.write('hello')
        assert FILENAME.read_text() == 'hello'

+    # Skip if we are on Windows and do not have admin rights
+    @unittest.skipIf(


Quibble: extract the common variable!

IS_WINDOWS_USER = os.name == 'nt' and not is_windows_admin() skip_windows = unittest.skipIf( IS_WINDOWS_USER, 'This test requires admin privileges to create symlink files on Windows' ) [....] @skip_windows def test_something():

I definitely learned something new here!

test/test_open.py

rec · 2024-11-02T13:01:58Z

Voila: #28

I'll add you as a reviewer once you've accepted my invitation on this repo...

rec

Really just textual issues, nothing big.

test/test_open.py

fix test_file_perms

bdc5fcc

sourcery-ai bot reviewed Oct 22, 2024

View reviewed changes

Chiemezuo added 2 commits October 22, 2024 17:22

switch 'is' to '=='

9a3c641

fix failing symlink file & dirs test

2394643

fix test_tempfile_perms

5e7a353

rec reviewed Oct 24, 2024

View reviewed changes

Chiemezuo added 3 commits October 25, 2024 07:46

change usage of bare except

8b41a44

implement file_perms test review

3df30d5

use @unittest.skipIf for pertinent tests

2208017

Chiemezuo requested a review from rec October 25, 2024 07:13

Chiemezuo added 3 commits October 25, 2024 16:56

fix unused variable e

28a9ac1

fix test_wrapper_bug2

f432696

fix test_wrapper_bug3

de1db77

rec reviewed Nov 2, 2024

View reviewed changes

fix quibbles from review by Tom

2cd60af

rec reviewed Nov 3, 2024

View reviewed changes

make minor syntax fixes

29d7871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Failing Tests on Windows (WIP) #27

Fix Failing Tests on Windows (WIP) #27

Chiemezuo commented Oct 22, 2024 •

edited

Loading

sourcery-ai bot commented Oct 22, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

sourcery-ai bot Oct 22, 2024

sourcery-ai bot Oct 22, 2024

Chiemezuo commented Oct 22, 2024

rec left a comment

rec Oct 24, 2024 •

edited

Loading

Chiemezuo Oct 25, 2024

Chiemezuo Oct 25, 2024

rec Oct 25, 2024

rec Oct 24, 2024

Chiemezuo Oct 25, 2024

Chiemezuo Oct 25, 2024

rec Oct 24, 2024

Chiemezuo Oct 25, 2024

rec Oct 24, 2024

Chiemezuo Oct 25, 2024

Chiemezuo commented Oct 25, 2024 •

edited

Loading

rec commented Oct 25, 2024

Chiemezuo commented Oct 31, 2024

rec Nov 2, 2024

Chiemezuo Nov 2, 2024

rec Nov 2, 2024

Chiemezuo Nov 2, 2024

rec commented Nov 2, 2024

rec left a comment

		@@ -196,6 +220,13 @@ def test_symlink_file(self, safer_open):

		def test_symlink_directory(self, safer_open):
		FILENAME = Path('sub/test.txt')

Fix Failing Tests on Windows (WIP) #27

Are you sure you want to change the base?

Fix Failing Tests on Windows (WIP) #27

Conversation

Chiemezuo commented Oct 22, 2024 • edited Loading

Summary by Sourcery

sourcery-ai bot commented Oct 22, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Oct 22, 2024

Choose a reason for hiding this comment

sourcery-ai bot Oct 22, 2024

Choose a reason for hiding this comment

Chiemezuo commented Oct 22, 2024

rec left a comment

Choose a reason for hiding this comment

rec Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chiemezuo commented Oct 25, 2024 • edited Loading

rec commented Oct 25, 2024

Chiemezuo commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rec commented Nov 2, 2024

rec left a comment

Choose a reason for hiding this comment

Chiemezuo commented Oct 22, 2024 •

edited

Loading

sourcery-ai bot commented Oct 22, 2024 •

edited

Loading

rec Oct 24, 2024 •

edited

Loading

Chiemezuo commented Oct 25, 2024 •

edited

Loading