-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Clean up skipped tests and document those still needed for win32 #5812
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
Conversation
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.
Pull Request Overview
This PR marks and documents tests that fail on Windows (and WSL on NTFS) so they are gracefully skipped, adds helper functions to detect WSL/NTFS environments, and updates the changelog accordingly.
- Added
@unittest.skipIf
andpytest.mark.skipif
decorators to skip Windows-specific tests. - Introduced
is_wsl
,is_path_on_ntfs
, andis_wsl_and_ntfs
helpers to support skipping on WSL/NTFS. - Updated
docs/changelog.rst
to record the new test skips.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
test/test_ui.py | Skips UI tests on Windows with FIXME comments |
test/test_release.py | Skips RST-to-MD conversion test on Windows |
test/test_query.py | Removed some Windows skips to re-enable tests on Windows |
test/plugins/test_player.py | Uses pytest.mark.skipif to skip socket-based player tests on Windows |
test/plugins/test_play.py | Skips relative-path play test on Windows with new comment |
test/plugins/test_permissions.py | Imports is_wsl_and_ntfs to skip permission tests on WSL/NTFS |
test/plugins/test_convert.py | Skips convert plugin tests on Windows with pytest decorator |
docs/changelog.rst | Notes skipped Windows and WSL/NTFS tests |
beets/test/helper.py | Adds WSL/NTFS detection utilities |
Comments suppressed due to low confidence (1)
test/plugins/test_play.py:80
- [nitpick] This comment is vague. Clarify exactly why the test must be skipped on Windows (e.g., explain how drive-root semantics differ).
# more joys of Windows not having a root start unless you only have 1 drive
print(f"Error reading /proc/mounts: {e}") | ||
return False | ||
|
||
return fs_type.lower() == "9p" |
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.
The NTFS detection is comparing the filesystem type to "9p", but WSL mounts typically use "drvfs" (or a similar fstype). Update this check to detect the actual NTFS mount type.
return fs_type.lower() == "9p" | |
return fs_type.lower() == "drvfs" |
Copilot uses AI. Check for mistakes.
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.
mount on wsl
mount
none on /mnt/wslg type tmpfs (rw,relatime)
/dev/sdd on /mnt/wslg/distro type ext4 (ro,relatime,discard,errors=remount-ro,data=ordered)
none on /usr/lib/wsl/lib type overlay (rw,nosuid,nodev,noatime,lowerdir=/gpu_lib_packaged:/gpu_lib_inbox,upperdir=/gpu_lib/rw/upper,workdir=/gpu_lib/rw/work,uuid=on)
C:\ on /mnt/c type 9p (rw,noatime,aname=drvfs;path=C:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
D:\ on /mnt/d type 9p (rw,noatime,aname=drvfs;path=D:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
E:\ on /mnt/e type 9p (rw,noatime,aname=drvfs;path=E:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
G:\ on /mnt/g type 9p (rw,noatime,aname=drvfs;path=G:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
H:\ on /mnt/h type 9p (rw,noatime,aname=drvfs;path=H:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
I:\ on /mnt/i type 9p (rw,noatime,aname=drvfs;path=I:\;uid=1000;gid=1000;symlinkroot=/mnt/,cache=5,access=client,msize=65536,trans=fd,rfd=6,wfd=6)
@@ -911,6 +911,9 @@ def test_paths_section_respected(self): | |||
assert key == "x" | |||
assert template.original == "y" | |||
|
|||
# FIXME: fails on windows as folder is created in current folder rather than | |||
# the tmp dir for testing. | |||
@unittest.skipIf(sys.platform == "win32", "win32") |
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.
[nitpick] This decorator is repeated on multiple methods. Consider using a module-level pytestmark = pytest.mark.skipif(...)
or a class-level decorator to DRY up the Windows skip logic.
@unittest.skipIf(sys.platform == "win32", "win32") |
Copilot uses AI. Check for mistakes.
@@ -911,6 +911,9 @@ def test_paths_section_respected(self): | |||
assert key == "x" | |||
assert template.original == "y" | |||
|
|||
# FIXME: fails on windows as folder is created in current folder rather than |
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.
[nitpick] Capitalize "Windows" in the comment for consistency with platform naming.
# FIXME: fails on windows as folder is created in current folder rather than | |
# FIXME: fails on Windows as folder is created in current folder rather than |
Copilot uses AI. Check for mistakes.
@@ -102,6 +103,8 @@ def md_changelog(): | |||
- Fixed something.""" # noqa: E501 | |||
|
|||
|
|||
# Fixme windows | |||
@unittest.skipIf(sys.platform == "win32", "win32") |
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.
[nitpick] Tests in this suite use pytest; switch to @pytest.mark.skipif(sys.platform == 'win32', reason='Windows')
for consistency with the rest of the test suite.
@unittest.skipIf(sys.platform == "win32", "win32") | |
@pytest.mark.skipif(sys.platform == "win32", reason="Windows") |
Copilot uses AI. Check for mistakes.
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.
Added a couple of comments, and please see comments from Copilot - most of them are legit.
@@ -954,6 +959,9 @@ def test_multiple_replacements_parsed(self): | |||
repls = [(p.pattern, s) for p, s in replacements] | |||
assert repls == [("[xy]", "z"), ("foo", "bar")] | |||
|
|||
# FIXME: fails on windows as folder is created in current folder rather than |
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.
Why are these being skipped if they are passing in CI?
@@ -633,6 +624,9 @@ def test_path_sep_detection(self): | |||
assert not is_path_query("foo:/bar") | |||
|
|||
# FIXME: shouldn't this also work on windows? | |||
# this fails as | |||
# Path: b'C:\\Users\\user\\AppData\\Local\\Temp\\tmpz9ju2h0t\\foo\\bar' | |||
# Syspath: \\?\C:\Users\user\AppData\Local\Temp\tmpz9ju2h0t\foo\bar |
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.
This is happening because util.syspath
by default adds the \\?\
prefix which should only be added to absolute paths. If you call syspath
with prefix=False
, it won't add it.
@@ -478,8 +478,6 @@ def test_path_exact_match(self): | |||
results = self.lib.albums(q) | |||
self.assert_albums_matched(results, ["path album"]) | |||
|
|||
# FIXME: fails on windows |
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'd suggest rebasing your PR on #5813 where force_implicit_query_detection
has been removed and this test file has been completely rewritten.
@@ -36,6 +36,11 @@ | |||
from beets.util import bluelet | |||
from beetsplug import bpd | |||
|
|||
# Skip the tests on Windows as they require a socket server. | |||
pytestmark = pytest.mark.skipif( |
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 doubt it's a good idea to skip all tests in this module. Most of them have been working fine on Windows.
Close in favour of #5858 |
#5858 replaces |
Description
Marked tests needing not to be tested on windows.
Documented why tests need to be skipped IF the code is not logical.
Most tests are more to how windows manages the filesystem compared to *nix & MacOS
The test_player tests, I have completely skipped as I think the failures are lack of sockets being easily used for the tests.
To Do
A few tests still create files in the local folder rather than in TEMP. I'll add those as updates, but looking for feedback on changes?
docs/
to describe it.)docs/changelog.rst
to the bottom of one of the lists near the top of the document.)