Repository navigation
fix(shared): update getSpotifyRelatedArtists tests for the #648 Last.fm fallback - #904
Closed
LucasSantana-Dev wants to merge 1 commit into
Closed
LucasSantana-Dev wants to merge 1 commit into
LucasSantana-Dev wants to merge 1 commit into
Conversation
…fm fallback PR #648 ("fix(artists): related-artists fallback + responsive panel + batch save endpoint") rewired `getSpotifyRelatedArtists` after Spotify deprecated `/v1/recommendations` and `/v1/artists/{id}/related-artists` for new apps (2024). The new impl chains: 1. GET https://api.spotify.com/v1/artists/{id} → seed artist name 2. GET https://ws.audioscrobbler.com/2.0/?... → similar names (Last.fm) 3. GET https://api.spotify.com/v1/search?... → per-name lookup The tests stayed mocking the old `/v1/recommendations` payload shape (`{ tracks: [{ artists: [...] }] }`) with a single fetch. The new impl's first call is `fetchSpotifyArtistName`, which the old mocks don't satisfy; the impl early-returns `[]` before any of the assertions can match. Three tests have been red since #648: - `should return unique artists from recommendations` - `should deduplicate duplicate artist ids in recommendations` - `should limit results to 12 artists` This commit: - Adds three response-builder helpers (`makeSpotifyArtistNameResponse`, `makeLastFmSimilarResponse`, `makeSpotifySearchResponse`) so the chain's fetch sequence is readable per test. - Sets `process.env.LASTFM_API_KEY` in `beforeEach` and restores it in `afterEach`; without it, `fetchLastFmSimilarArtists` early-returns `[]`. - Rewrites the first two failing tests to mock the full Spotify → Last.fm → Spotify search chain instead of the dead `/v1/recommendations` payload. - Replaces the `should limit results to 12 artists` test with one that asserts the actual current contract: the impl returns all unique resolved artists from Last.fm (no artificial cap below the `Math.max(limit, 30)` lookup bound). The pre-#648 12-cap is gone; a regression that re-introduces a silent cap would fail this test. Verification: - `npx jest --config packages/shared/jest.config.cjs --ci packages/shared/src/__tests__/utils/spotify/artistApi.test.ts`: 9/9 passing. - `npm run test:ci --workspace=packages/shared` against this branch: 417/417 tests passing. The one remaining failed suite (`FeatureToggleService.spec.ts`) is fixed by the in-flight PR #903 and out of scope here. No impl change. Phase 1 loop = the spec itself (deterministic, 2-second run). H1 confirmed at the start of the diagnostic session: tests stale vs new impl. No regression-test seam was needed (we are bringing existing tests back into alignment with existing correct behavior). Refs: PR #648 (introduced the chain), this PR (#904).
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Failed to generate code suggestions for PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe test suite for ChangesSpotify Artist API Test Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This branch was successfully deployed
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
1 participant
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.
Summary
Three tests in
packages/shared/src/__tests__/utils/spotify/artistApi.test.tshave been red since PR #648 (Spotify→Last.fm fallback rewrite). The impl was updated; the tests weren't. This PR brings the tests back into alignment with current behavior.Diagnostic trail
Followed
/diagnosediscipline. Phase 1 loop = the spec itself (2-sec deterministic). Phase 2 reproduced: all three failing tests early-return[]because the impl's firstfetch(fetchSpotifyArtistName) isn't mocked by the existing old-shape mock. Phase 3 ranked four hypotheses; H1 (stale tests) confirmed against H2 (impl regressed) and H3 (dead code —backend/src/routes/artists.ts:404is a live consumer).What changed
makeSpotifyArtistNameResponse,makeLastFmSimilarResponse,makeSpotifySearchResponse) make each test's fetch sequence legible.process.env.LASTFM_API_KEYis set inbeforeEachand restored inafterEach; without itfetchLastFmSimilarArtistsearly-returns[].should return unique artists from recommendations) → mocks the full Spotify → Last.fm → Spotify search chain.should deduplicate duplicate artist ids in recommendations) → mocks two similar names that resolve to the same Spotify id; asserts the dedupe by id still holds.should limit results to 12 artists) → replaced. The pre-fix(artists): related-artists fallback + responsive panel + batch save endpoint #648 impl had a 12-cap; the post-fix(artists): related-artists fallback + responsive panel + batch save endpoint #648 impl returns all unique resolved artists up to theMath.max(limit, 30)lookup bound. The new test name and body assert the actual contract and guard against a silent cap regression.The two already-passing tests (403 graceful, empty response) are kept; they happen to pass because the impl's first fetch returns null in those cases, which is a real behavior worth keeping under test.
Verification
npx jest --config packages/shared/jest.config.cjs --ci packages/shared/src/__tests__/utils/spotify/artistApi.test.ts: 9/9 passing.npm run test:ci --workspace=packages/shared: 417/417 tests passing on this branch. The single remaining failed suite (FeatureToggleService.spec.ts) is fixed by in-flight PR fix(shared,backend,db): retire orphan per-guild feature toggle substrate #903 and out of scope here.Notes
getSpotifyRelatedArtistsis correct; only the tests were lying about its contract.Test plan
Quality Gateson this PRRefs: PR #648 (introduced the chain), #903 (sibling test-cleanup PR open against same base).
Summary by CodeRabbit