Make api_request's HTTP client injectable + add request-shape tests#196
Merged
Conversation
Until now lib/utils/api_request.dart called the top-level Http.get / post / put / patch / delete helpers from package:http directly, so nothing in the test suite could observe the URL, method, headers or body the player actually sends. The 422 bug in AlbumProvider.toggleFavorite (where we sent type: 'albums' and the koel server expects type: 'album') slipped through for exactly that reason — there was no place to write a regression test against the request shape. This commit: - Adds a module-level `_client` to api_request.dart, defaulting to a fresh http.Client(), and routes every per-method helper through it. - Exposes setHttpClientForTesting / resetHttpClientForTesting (annotated @VisibleForTesting) so tests can swap in a MockClient from package:http/testing.dart. - Adds api_request_test.dart covering URL building, method dispatch for GET/POST/PUT/PATCH/DELETE, header shape (content-type, accept, X-Api-Version, optional bearer), and 2xx / non-JSON / non-2xx response paths. - Adds toggle_favorite_request_shape_test.dart that pins the singular type strings the koel server's FavoriteableType enum requires — album, artist, podcast, radio-station — for all four providers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces direct static HTTP calls with a module-level, overridable ChangesHTTP client injection + test harness and provider tests
Sequence Diagram(s)sequenceDiagram
participant Provider
participant ApiLayer as api_request.dart
participant HttpClient as Http.Client (in tests: CapturingClient)
participant Server
Provider->>ApiLayer: call api.get/post/put/patch/delete(path, data?)
ApiLayer->>HttpClient: _client.<verb>(uri, headers, body?)
HttpClient->>Server: HTTP request (captures method, url, headers, body)
Server-->>HttpClient: HTTP response (status, body)
HttpClient-->>ApiLayer: Response
ApiLayer-->>Provider: returns decoded JSON / null or throws HttpResponseException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 3 seconds.Comment |
Asserting that toggleFavorite POSTs type:album doesn't catch anything the source doesn't already say plainly. If someone typos the source they'll typo the test the same way — which is exactly how the original 422 bug would have been written and would have passed. Keep the actually-useful piece: the injectable http.Client and the api_request_test.dart that exercises real request/response behaviour.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/api_request_test.dart`:
- Around line 170-181: The test "throws HttpResponseException on non-2xx" is
currently passing a Future-returning closure to expect without awaiting it;
change the assertion to await the asynchronous expectation (e.g., use await
expectLater or await expect with the Future) so the test actually awaits
api.post and fails if no exception is thrown. Locate the test that sets up
_captureClient, calls api.setHttpClientForTesting(mock.client) and invokes
api.post('favorites/toggle', ...), and replace the synchronous expect(() =>
api.post(...), throwsA(isA<HttpResponseException>())) with an awaited async
expectation that directly awaits the Future returned by api.post and asserts
throwsA(isA<HttpResponseException>)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbd4e717-1683-471e-a109-77f90f9ec846
📒 Files selected for processing (3)
lib/utils/api_request.darttest/providers/toggle_favorite_request_shape_test.darttest/utils/api_request_test.dart
Behaviour tests for AlbumProvider, ArtistProvider, RadioStationProvider,
PodcastProvider, and PlayableProvider.fetchForPodcast — driven by a
shared MockClient harness so each test exercises the actual
request → response → state-mutation → notifyListeners pipeline.
For each operation the suite locks down:
- the URL, method, and JSON body the provider sends;
- the model fields the response merges into;
- the listener notification count;
- the rollback + rethrow shape on a non-2xx response, where the
provider does an optimistic mutation (toggleFavorite,
unsubscribePodcast).
The CapturingClient helper in test/helpers/api_test_setup.dart wires
in the path_provider mock + GetStorage init so every provider test
gets a clean preferences slate (per-isolate temp dir to avoid the
'./Preferences.gs' file lock contention when test files run in
parallel).
Net 25 new tests; 375 pass total.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/utils/api_request_test.dart (1)
129-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAwait the async throw assertion in the non-2xx test
This still uses a non-awaited async expectation pattern; use
await expectLater(...)to ensure the test actually fails on regression.Proposed fix
- expect( - () => api.post('favorites/toggle', data: {}), - throwsA(isA<HttpResponseException>()), - ); + await expectLater( + api.post('favorites/toggle', data: {}), + throwsA(isA<HttpResponseException>()), + );#!/bin/bash # Verify the assertion style used in the non-2xx test. rg -n -C3 "throws HttpResponseException on non-2xx|expect\\(|expectLater\\(" test/utils/api_request_test.dart🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/api_request_test.dart` around lines 129 - 137, The test "throws HttpResponseException on non-2xx" currently uses a non-awaited expect with a closure; change it to await the async assertion by replacing the expect(() => api.post('favorites/toggle', data: {}), throwsA(isA<HttpResponseException>())) with await expectLater(api.post('favorites/toggle', data: {}), throwsA(isA<HttpResponseException>())). This uses the existing CapturingClient and HttpResponseException symbols and ensures the Future returned by api.post is awaited by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/utils/api_request_test.dart`:
- Around line 129-137: The test "throws HttpResponseException on non-2xx"
currently uses a non-awaited expect with a closure; change it to await the async
assertion by replacing the expect(() => api.post('favorites/toggle', data: {}),
throwsA(isA<HttpResponseException>())) with await
expectLater(api.post('favorites/toggle', data: {}),
throwsA(isA<HttpResponseException>())). This uses the existing CapturingClient
and HttpResponseException symbols and ensures the Future returned by api.post is
awaited by the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d91896b7-a1aa-4d7d-a1e8-52d2bcf6a046
📒 Files selected for processing (7)
test/helpers/api_test_setup.darttest/providers/album_provider_test.darttest/providers/artist_provider_test.darttest/providers/playable_provider_test.darttest/providers/podcast_provider_test.darttest/providers/radio_station_provider_test.darttest/utils/api_request_test.dart
expect(closure, throwsA(...)) on a Future-returning closure resolves asynchronously. Without awaiting, the test body returns before the assertion completes — so a regression where the throw stops happening would pass silently. Switch to await expectLater so the test actually waits for the throw.
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
Closes a long-standing testing gap:
lib/utils/api_request.dartcalled the top-levelHttp.get / post / put / patch / deletehelpers frompackage:httpdirectly, so nothing in the test suite could observe the URL, method, headers or body the player actually sends. The 422 bug inAlbumProvider.toggleFavorite(where we senttype: 'albums'and the koel server's `FavoriteableType` enum expectstype: 'album') slipped through for exactly that reason — there was no place to write a regression test for request shape.This PR:
No production-code behaviour change: the default client is identical to the old direct calls. Every existing caller (`get/post/put/patch/delete` top-level functions) is unchanged.
Test plan
Summary by CodeRabbit