Skip to content

[Test] Fix path tests on windows #5803

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

Merged
merged 4 commits into from
May 26, 2025

Conversation

citelao
Copy link
Contributor

@citelao citelao commented May 25, 2025

Description

Fixes #5802.

Today, tests fail on most Windows machines because we hard-code D: as the root drive, but most machines use C:. This change uses the same normalization function in the test assertion to ensure the drives match.

To Do

  • Documentation.
  • Changelog.
  • Tests. (this is a tests change)

What changed?

  • Updated tests to generate the drive name via normalization, instead of hard-coding D:.
  • Updated the Item::destination() method to document the relative_to_libdir param.

How tested?

  • Tests pass locally.

@Copilot Copilot AI review requested due to automatic review settings May 25, 2025 19:35
Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Copy link
Contributor

@Copilot Copilot AI left a 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 pull request fixes Windows path tests by replacing the hard-coded drive letter "D:" with the system drive value fetched dynamically from the environment.

  • Replaces hard-coded "D:" with os.environ.get("SystemDrive", "C:") in test assertions.
  • Improves documentation of the destination method in the library module.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/test_library.py Updates to Windows path handling in tests and declaration of instance variables.
beets/library.py Updates the docstring to clarify behavior when the relative_to_libdir parameter is true.

@citelao
Copy link
Contributor Author

citelao commented May 25, 2025

Hmm... this D:/ seems to be unrelated to the system drive. I bet it's a specific drive for Github Actions...

@citelao
Copy link
Contributor Author

citelao commented May 25, 2025

Hmm... this D:/ seems to be unrelated to the system drive. I bet it's a specific drive for Github Actions...

Root-caused: Item::destination() relies on util.normpath, which calls os.path.abspath, which relies on your current working directory (cwd). I bet the cwd in our tests happens to be D:.

@semohr
Copy link
Contributor

semohr commented May 26, 2025

Thank you for the quick fix and find!

Funny that the drive was hardcoded, I guess not too many people run the tests on a local windows machine.

Could you add a changelog entry? Other than that, should be ready to merge.

@semohr semohr added testing Relates to the testing/CI infrastructure windows Relates specifically to Windows OS labels May 26, 2025
@citelao
Copy link
Contributor Author

citelao commented May 26, 2025

Happy to help! Changelog pushed.

@semohr semohr merged commit da5ec00 into beetbox:master May 26, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Relates to the testing/CI infrastructure windows Relates specifically to Windows OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dev] ~50 library tests fail with the wrong drive on Windows
3 participants