tests(windows): fix remaining "unixisms" that break some tests

[kiike]

When I was developing an earlier PR, I wanted to test it on a vanilla installation of Windows, just to make sure no config could pollute the results.

Turns out, some tests fail on this kind of setup. Even with a benign but creepy Windows error popup during tests/commands/open.

Some reasons for the failing tests:

• symlinks no functional unless "developer mode is active" (I guess CI never sees this because it's running a modified build)
• typical unix commands such as mv, echo or mv not present in vanilla Windows.

Some details of this PR:

• It actually contains a fix that is present on a commit of feat(rename): allow batch and auto-renaming from pattern #810, so I'll remove it from there.
• I want to explain the user why symlinks don't work if we get an exception.
• I'm sad to make ["git"] if git else []) + ["mv", folder, new_folder_path] go, being so clever. I hope using shutils is OK.
• the add via symlink test is marked as xfail, just in case it succeeds.
• script.py now has ls too.

Looking forward to your feedback! I think this PR getting in will make it a bit more polished when a new user tries the 0.14 release on Windows, wants to contribute, and tests pass on a fresh install!

[kiike]

I misunderstood the Python docs and thought I could have WindowsError in the code and that it wouldn't just check, but that actually breaks on Linux as it is undefined. I changed it to the more generic OSError and then a guard to check for the actual error code.

[alexfikl]

This is great! 🎉

Left a few questions and nitpicks, but hopefully easy to fix.

I misunderstood the Python docs and thought I could have WindowsError in the code and that it wouldn't just check, but that actually breaks on Linux as it is undefined. I changed it to the more generic OSError and then a guard to check for the actual error code.

From the docs, WindowsError also seems to be deprecated since version 3.3, so we should just use OSError: https://docs.python.org/3/library/exceptions.html#WindowsError.

[alexfikl]

Does "start" not work here? Or?

[kiike]

It raises WinError, an exception that subclasses OSError. WinError can’t be in code that would run on Linux, since that’s undefined. I’ll probably move Windows specific stuff into its own utility function as you suggested so that we have specific WinError exceptions. As for checking before actually running the operations, I guess that’s not impossible, but where should the check go? After installation, warning the user that functionality would be reduced due to dev mode not enabled, for instance? Enric Morales ***@***.***>

________________________________ From: Alex Fikl ***@***.***> Sent: Saturday, April 13, 2024 8:25:06 AM To: papis/papis ***@***.***> Cc: Enric Morales ***@***.***>; Author ***@***.***> Subject: Re: [papis/papis] tests(windows): fix remaining "unixisms" that break some tests (PR #816) @alexfikl commented on this pull request.

________________________________ In papis/commands/add.py<#816 (comment)>:

+ try:

+ logger.info("[SYMLINK] '%s' to '%s'.", in_file_abspath, + tmp_end_filepath) + os.symlink(in_file_abspath, tmp_end_filepath) + except OSError as e: + missing_privilege_error = 1314 + if sys.platform == "win32" and e.winerror == missing_privilege_error: + logger.error("Failed to link due to insufficient permissions. You " + "can try again after enabling the 'Developer mode' " + "and restarting.") + raise I can't check, but does this throw just a generic OSError? Maybe we can better check for a PermissionError or something to show that message. — Reply to this email directly, view it on GitHub<#816 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AADRN4PM2PI473SQRYSQE6DY5DFUFAVCNFSM6AAAAABGEMMQS2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJZGA2TOOBRGI>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[alexfikl]

It raises WinError, an exception that subclasses OSError. WinError can’t be in code that would run on Linux, since that’s undefined. I’ll probably move Windows specific stuff into its own utility function as you suggested so that we have specific WinError exceptions.

According to the docs, WindowsError is an alias of OSError starting with Python 3.3, not a subclass of it. But yeah, it's not defined at all on non-Windows systems, so it can't be used.

As for checking before actually running the operations, I guess that’s not impossible, but where should the check go? After installation, warning the user that functionality would be reduced due to dev mode not enabled, for instance?

I was just thinking for that specific test, to just add a little

if not is_win_dev_mode():
    pytest.skip("This test requires Developer Mode on Windows")

But it was mostly for curiosity. They way it is now is just fine!

[kiike]

Blocked until #810 gets in.

[alexfikl]

Left a few more suggestions, but this should be good to go once we can use the git stuff from #810.

[kiike]

I'm checking the exception on windows, to see if it is raising with all arguments

EDIT: It works perfectly, with the files that failed to link and all.

[alexfikl]

@kiike Looks good to me now! Ready to go?

[kiike]

@kiike Looks good to me now! Ready to go?

Yes! Let's do it! But if you want me to clean up the commit list so they are logically separated by what they want to do, I can do it to ease reverting/tracking when was something introduced. Just tell me or squash merge, I don't mind 😄

[alexfikl]

@kiike Looks good to me now! Ready to go?

Yes! Let's do it! But if you want me to clean up the commit list so they are logically separated by what they want to do, I can do it to ease reverting/tracking when was something introduced. Just tell me or squash merge, I don't mind 😄

I don't know, squashing should be fine. My "rule" is just that when there's some "fix this and that" commits from the review => squash it 😁

Thank you for working on this! In it went 🚀