Tidy up the duplicated Tidal test fixtures - #5459
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the provider-test fixture consolidation work by unifying the previously duplicated Tidal provider_mock fixtures into a single shared implementation in tests/providers/tidal/conftest.py, so all Tidal provider tests start from the same fully wired mock provider.
Changes:
- Centralizes the common Tidal
provider_mock(auth/session, api, mass collaborators,get_item_mapping, pagination helpers) intotests/providers/tidal/conftest.py. - Removes per-test-module
provider_mockduplicates across the Tidal test suite. - Keeps a streaming-specific customization in
test_streaming.py(quality + throttler bypass), intended to be layered on top of the shared mock.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/providers/tidal/conftest.py | Introduces a shared, fully wired provider_mock and keeps shared throttling patches. |
| tests/providers/tidal/test_streaming.py | Replaces the local provider mock with a streaming-specific customization of the shared mock. |
| tests/providers/tidal/test_recommendations.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_playlist.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_parsers.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_page_parser.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_page_parser_extended.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_media.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_media_extended.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_library.py | Drops module-local provider_mock in favor of the shared fixture. |
| tests/providers/tidal/test_api_client.py | Drops module-local provider_mock in favor of the shared fixture. |
marcelveldt
force-pushed
the
optimistic-kepler-90e13d
branch
from
August 7, 2026 17:32
571b1db to
608cfd6
Compare
marcelveldt
force-pushed
the
optimistic-kepler-90e13d
branch
from
August 7, 2026 17:40
608cfd6 to
277d228
Compare
marcelveldt
marked this pull request as ready for review
August 7, 2026 19:22
17 tasks
marcelveldt
added a commit
that referenced
this pull request
Aug 7, 2026
# What does this implement/fix? A Tidal test checked that the ISRC lookup is sent to the Tidal open API, but it compared the URL against a value the test fixture had made up itself rather than the URL the provider actually uses. The check passed only because the two strings happened to be identical, so it would not have noticed if the lookup stopped targeting the open API. The test now compares against the provider's real `OPEN_API_URL` constant. - Import `OPEN_API_URL` from the Tidal provider constants in `tests/providers/tidal/test_streaming.py` and assert against it - Drop the unused `provider_mock.api.OPEN_API_URL` line from the fixture (production code never reads that attribute) Verified by removing `base_url` from the lookup call in the provider: the test now fails, where before it passed. Follow-up to #5459, which left this alone to keep that PR behaviour-neutral. ## Types of changes - [ ] Bugfix (non-breaking change which fixes an issue) — `bugfix` - [ ] New feature (non-breaking change which adds functionality) — `new-feature` - [ ] Enhancement to an existing feature — `enhancement` - [ ] New music/player/metadata/plugin provider — `new-provider` - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) — `breaking-change` - [ ] Refactor (no behaviour change) — `refactor` - [ ] Documentation only — `documentation` - [x] Maintenance / chore — `maintenance` - [ ] CI / workflow change — `ci` - [ ] Dependencies bump — `dependencies` ## Checklist - [x] The code change is tested and works locally. - [x] `pre-commit run --all-files` passes. - [x] `pytest` passes, and tests have been added/updated under `tests/` where applicable. - [x] For changes to shared models, the companion PR in `music-assistant/models` is linked. - [x] For changes affecting the UI, the companion PR in `music-assistant/frontend` is linked. - [x] I have read and complied with the project's [AI Policy](https://github.com/music-assistant/.github/blob/main/AI_POLICY.md) for any AI-assisted contributions. - [x] I have [raised a PR against the documentation repository](https://github.com/music-assistant/music-assistant.io/blob/main/CONTRIBUTING.md) targeting the main or beta branch as appropriate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this implement/fix?
Follow-up to #5452, which left alone the fixtures that "merely share a name but differ in body". The Tidal tests were the worst case: ten test files each defined their own
provider_mock, all variations on the same thing, and some missing bits the others had.That became a trap once #5452 hoisted
media_managerinto the directory'sconftest.py, because that fixture depends onprovider_mockand the conftest doesn't define one. It resolves from whichever module asks for it, so adding amedia_managertest totest_playlist.pywould silently get a mock with noget_item_mappingand fail on an unreadable<Mock name='mock.get_item_mapping().item_id'>comparison instead of a clear error.The ten copies now merge into one shared
provider_mockin the directory'sconftest.py, somedia_manageris self-contained and every module starts from the same fully-wired provider.provider_mockintests/providers/tidal/conftest.py, merged from the ten module copiestest_streaming.pykeeps a small same-name override for the parts only it needs (streaming quality and the throttler bypass)media_managerexplaining the cross-module resolution is no longer neededRelated issue (if applicable):
Types of changes
bugfixnew-featureenhancementnew-providerbreaking-changerefactordocumentationmaintenancecidependenciesChecklist
pre-commit run --all-filespasses.pytestpasses, and tests have been added/updated undertests/where applicable.music-assistant/modelsis linked.music-assistant/frontendis linked.