Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the test suite by replacing the deprecated tmpdir fixture with the newer tmp_path fixture across all test files. The tmp_path fixture is the recommended approach in pytest as it returns a pathlib.Path object instead of a legacy py.path.local object, providing better type safety and consistency with modern Python path handling.
Key changes:
- Replaced
tmpdirparameter withtmp_pathin all test function signatures - Updated all references from
tmpdirtotmp_pathwithin test function bodies
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/test_metadata.py | Updated fixture parameter and internal references in test_embed_metadata |
| tests/utils/test_m3u.py | Updated fixture parameter and internal references in test_create_m3u_file |
| tests/utils/test_github.py | Updated fixture parameter and internal references in test_download_github_dir |
| tests/utils/test_ffmpeg.py | Updated fixture parameter and internal references in test_download_ffmpeg and test_convert |
| tests/utils/test_config.py | Updated fixture parameter and internal references in setup fixture |
| tests/utils/test_archive.py | Updated fixture parameter and internal references in test_load_archive |
| tests/test_init.py | Updated fixture parameter and internal references in test_download |
| tests/console/test_entry_point.py | Updated fixture parameter and internal references in test_download_song and test_preload_song |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| playlist = Playlist.from_url(PLAYLIST) | ||
| create_m3u_file("test.m3u", playlist.songs, "", "mp3") | ||
| assert tmpdir.join("test.m3u").isfile() is True | ||
| assert tmp_path.join("test.m3u").isfile() is True |
There was a problem hiding this comment.
tmp_path is a pathlib.Path object and does not have a join() method. Use the / operator instead: assert (tmp_path / "test.m3u").is_file() is True. Also note that pathlib.Path uses is_file() instead of isfile().
| assert tmp_path.join("test.m3u").isfile() is True | |
| assert (tmp_path / "test.m3u").is_file() is True |
| download_dir = tmp_path.listdir()[0] | ||
| assert download_dir.isdir() is True | ||
| assert download_dir.join("index.html").isfile() is True |
There was a problem hiding this comment.
tmp_path is a pathlib.Path object and does not have listdir(), isdir(), or join() methods. Use list(tmp_path.iterdir()) to list contents, is_dir() to check if it's a directory, and the / operator for path joining: download_dir = list(tmp_path.iterdir())[0], assert download_dir.is_dir() is True, and assert (download_dir / "index.html").is_file() is True.
| download_dir = tmp_path.listdir()[0] | |
| assert download_dir.isdir() is True | |
| assert download_dir.join("index.html").isfile() is True | |
| download_dir = list(tmp_path.iterdir())[0] | |
| assert download_dir.is_dir() is True | |
| assert (download_dir / "index.html").is_file() is True |
| archive1 = Archive(["a", "b", "c"]) | ||
| assert archive1.save("archive.txt") is True | ||
| assert tmpdir.join("archive.txt").isfile() is True | ||
| assert tmp_path.join("archive.txt").isfile() is True |
There was a problem hiding this comment.
tmp_path is a pathlib.Path object and does not have a join() method. Use the / operator instead: assert (tmp_path / "archive.txt").is_file() is True. Also note that pathlib.Path uses is_file() instead of isfile().
| assert tmp_path.join("archive.txt").isfile() is True | |
| assert (tmp_path / "archive.txt").is_file() is True |
Replace the tmpdir fixture with tmp_path
Description
Related Issue
Motivation and Context
see https://stackoverflow.com/a/36071167
How Has This Been Tested?
Screenshots (if appropriate)
Types of Changes
Checklist