Partial Implementation for Automated Software Tests #509
Conversation
WalkthroughAdds CI to run Flutter tests on PR open and enables tests in Android deploy workflow. Refactors several controllers to use constructor-based dependency injection. Adjusts ResonateUser JSON deserialization for userRating. Removes a build id. Adds comprehensive unit/widget tests with Mockito-generated mocks; deletes the default Flutter counter test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant WF as RUN_SOFTWARE_TESTS
participant R as GitHub Runner
Dev->>GH: Open Pull Request
GH-->>WF: trigger (pull_request_target: opened)
WF->>R: Start job (ubuntu-latest)
R->>R: actions/checkout@v4
R->>R: actions/setup-java@v4 (Zulu JDK 17.0.12)
R->>R: subosito/flutter-action@v2 (Flutter 3.32.5)
R->>R: flutter pub get
R->>R: flutter test
R-->>GH: Report test results/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🎉 Welcome @M4dhav!
We appreciate your contribution! 🚀 |
M4dhav
added
enhancement
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
M4dhav
changed the title
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (17)
lib/controllers/create_room_controller.dart (1)
105-107: Regex validation bug: contains() with anchored pattern yields false positives.Using String.contains with ^...$ allows substring matches, incorrectly validating inputs like "Football$". Use RegExp.hasMatch against the entire string instead.
Apply this fix:
- if (!hashtag.contains(RegExp(r'^[a-zA-Z0-9_]+$'))) { + if (!RegExp(r'^[a-zA-Z0-9_]+$').hasMatch(hashtag)) {lib/controllers/explore_story_controller.dart (3)
187-192: Bug: bucketId parameter ignored when creating file.storage.createFile always uses storyBucketId, ignoring the bucketId argument. This is error-prone and breaks the function’s contract.
- await storage.createFile( - bucketId: storyBucketId, + await storage.createFile( + bucketId: bucketId, fileId: fileId, file: InputFile.fromPath(path: filePath), );
544-549: Broken color-to-hex conversion (nonexistent properties a/r/g/b).Flutter’s Color exposes alpha, red, green, blue (ints), not a/r/g/b, and no 0..1 scaling. This won’t compile or will be incorrect.
- String toHex({bool leadingHashSign = true}) => '${leadingHashSign ? '#' : ''}' - '${(a * 255).toInt().toRadixString(16).padLeft(2, '0')}' - '${r.toInt().toRadixString(16).padLeft(2, '0')}' - '${g.toInt().toRadixString(16).padLeft(2, '0')}' - '${b.toInt().toRadixString(16).padLeft(2, '0')}'; + String toHex({bool leadingHashSign = true}) => '${leadingHashSign ? '#' : ''}' + '${alpha.toRadixString(16).padLeft(2, '0')}' + '${red.toRadixString(16).padLeft(2, '0')}' + '${green.toRadixString(16).padLeft(2, '0')}' + '${blue.toRadixString(16).padLeft(2, '0')}';
419-456: Potential crash when unliking without a like document.userLikeDocuments.first will throw if the query returns empty (e.g., due to race conditions). Guard for emptiness and only update likes after a successful delete; also no-op if nothing to delete.
- try { - await databases.deleteDocument( - databaseId: storyDatabaseId, - collectionId: likeCollectionId, - documentId: userLikeDocuments.first.$id, - ); - } on AppwriteException catch (e) { - log('Failed to Unlike i.e delete Like Document: ${e.message}'); - } - - try { - await databases.updateDocument( - databaseId: storyDatabaseId, - collectionId: storyCollectionId, - documentId: story.storyId, - data: {"likes": story.likesCount.value - 1}); - } on AppwriteException catch (e) { - log("Failed to reduce one story like: ${e.message}"); - } + if (userLikeDocuments.isEmpty) { + log('No like document found to delete for storyId=${story.storyId}'); + return; + } + try { + await databases.deleteDocument( + databaseId: storyDatabaseId, + collectionId: likeCollectionId, + documentId: userLikeDocuments.first.$id, + ); + // Only decrement likes after successful delete + try { + await databases.updateDocument( + databaseId: storyDatabaseId, + collectionId: storyCollectionId, + documentId: story.storyId, + data: {"likes": story.likesCount.value - 1}); + } on AppwriteException catch (e) { + log("Failed to reduce one story like: ${e.message}"); + } + } on AppwriteException catch (e) { + log('Failed to Unlike i.e delete Like Document: ${e.message}'); + }lib/controllers/change_email_controller.dart (3)
53-75: Consider handling partial-update inconsistency between the two collectionsIf the first update succeeds (users) and the second (username-email) fails, data becomes inconsistent. Appwrite doesn’t provide cross-document transactions, but you can:
- Update the username-email doc first, then the users doc, or
- Add compensating logic to revert the first update if the second fails, or
- Log and surface a specific error to instruct the user to retry.
At minimum, reorder the operations to update the secondary mapping first, which is safer to roll forward/back.
94-97: Surface errors to the user on unexpected exceptionsThe generic catch logs and returns false but provides no user feedback. Add a fallback snackbar + accessibility announcement for unexpected failures.
Apply this diff:
} catch (e) { log(e.toString()); - return false; + customSnackbar( + AppLocalizations.of(context)!.tryAgain, + e.toString(), + LogType.error, + ); + SemanticsService.announce( + e.toString(), + TextDirection.ltr, + ); + return false; }
143-200: Refactor nested futures to async/await and guarantee loading resetThe nested .then chains are hard to read and risk leaving isLoading = true on early returns/errors. Use async/await and a finally to ensure deterministic loading-state handling.
Apply this diff:
- Future<void> changeEmail(BuildContext context) async { - if (changeEmailFormKey.currentState!.validate()) { - isLoading.value = true; - - isEmailAvailable(emailController.text).then((status) { - if (status) { - changeEmailInAuth( - emailController.text, passwordController.text, context) - .then((status) { - if (status) { - changeEmailInDatabases(emailController.text, context) - .then((status) { - isLoading.value = false; - - if (status) { - customSnackbar( - AppLocalizations.of(context)!.emailChanged, - AppLocalizations.of(context)!.emailChangeSuccess, - LogType.success, - ); - SemanticsService.announce( - AppLocalizations.of(context)!.emailChangeSuccess, - TextDirection.ltr, - ); - } else { - customSnackbar( - AppLocalizations.of(context)!.failed, - AppLocalizations.of(context)!.emailChangeFailed, - LogType.error, - ); - SemanticsService.announce( - AppLocalizations.of(context)!.emailChangeFailed, - TextDirection.ltr, - ); - } - }); - } else { - isLoading.value = false; - } - }); - } else { - customSnackbar( - AppLocalizations.of(context)!.oops, - AppLocalizations.of(context)!.emailExists, - LogType.error, - ); - SemanticsService.announce( - AppLocalizations.of(context)!.emailExists, - TextDirection.ltr, - ); - isLoading.value = false; - } - }); - } - } + Future<void> changeEmail(BuildContext context) async { + if (!changeEmailFormKey.currentState!.validate()) return; + isLoading.value = true; + try { + final available = await isEmailAvailable(emailController.text); + if (!available) { + customSnackbar( + AppLocalizations.of(context)!.oops, + AppLocalizations.of(context)!.emailExists, + LogType.error, + ); + SemanticsService.announce( + AppLocalizations.of(context)!.emailExists, + TextDirection.ltr, + ); + return; + } + + final authOk = await changeEmailInAuth( + emailController.text, + passwordController.text, + context, + ); + if (!authOk) return; + + final dbOk = await changeEmailInDatabases( + emailController.text, + context, + ); + if (dbOk) { + customSnackbar( + AppLocalizations.of(context)!.emailChanged, + AppLocalizations.of(context)!.emailChangeSuccess, + LogType.success, + ); + SemanticsService.announce( + AppLocalizations.of(context)!.emailChangeSuccess, + TextDirection.ltr, + ); + } else { + customSnackbar( + AppLocalizations.of(context)!.failed, + AppLocalizations.of(context)!.emailChangeFailed, + LogType.error, + ); + SemanticsService.announce( + AppLocalizations.of(context)!.emailChangeFailed, + TextDirection.ltr, + ); + } + } finally { + isLoading.value = false; + } + }lib/controllers/edit_profile_controller.dart (5)
95-99: Guard Get.back() to avoid popping when no dialog/route is openCalling Get.back() unconditionally can throw if no route can pop (especially in tests). Guard with Get.isDialogOpen.
Apply this diff in both finally blocks:
- // Close the loading dialog - Get.back(); + // Close the loading dialog if open + if (Get.isDialogOpen == true) { + Get.back(); + }Also applies to: 131-134
141-153: Avoid null Get.context access inside _cropImageGet.context may be null; dereferencing it will throw. Compute a safe title with a fallback.
Apply this diff:
- AndroidUiSettings( - toolbarTitle: AppLocalizations.of(Get.context!)!.cropImage, + AndroidUiSettings( + toolbarTitle: (Get.context != null + ? AppLocalizations.of(Get.context!)?.cropImage + : null) ?? + 'Crop Image', // toolbarColor: themeController.primaryColor.value, // statusBarColor: themeController.primaryColor.value, // toolbarWidgetColor: Colors.black, // cropFrameColor: Colors.white, // activeControlsWidgetColor: themeController.primaryColor.value, ), IOSUiSettings( minimumAspectRatio: 1.0, - title: AppLocalizations.of(Get.context!)!.cropImage, + title: (Get.context != null + ? AppLocalizations.of(Get.context!)?.cropImage + : null) ?? + 'Crop Image', ),
209-217: Null-safe deletion of previous profile image fileprofileImageID may be null/empty; guard before delete to avoid runtime exceptions.
Apply this diff:
- await storage.deleteFile( - bucketId: userProfileImageBucketId, - fileId: authStateController.profileImageID!); + final oldId = authStateController.profileImageID; + if (oldId != null && oldId.isNotEmpty) { + await storage.deleteFile( + bucketId: userProfileImageBucketId, + fileId: oldId, + ); + }
159-171: Differentiate “not found” from other errors when checking username availabilityTreating any exception as “available” can approve usernames during transient failures. Inspect the AppwriteException and only return true on 404/not-found.
Apply this diff:
Future<bool> isUsernameAvailable(String username) async { try { await databases.getDocument( databaseId: userDatabaseID, collectionId: usernameCollectionID, documentId: username, ); return false; - } catch (e) { - log(e.toString()); - return true; + } on AppwriteException catch (e) { + // 404 means document not found => username available + if (e.code == 404) { + return true; + } + log(e.toString()); + return false; + } catch (e) { + log(e.toString()); + return false; } }
38-43: Dispose TextEditingControllers to avoid leaksimageController, nameController, and usernameController are never disposed. Dispose them in onClose().
Apply this diff near the end of the class:
} } + + @override + void onClose() { + imageController.dispose(); + nameController.dispose(); + usernameController.dispose(); + super.onClose(); + }Also applies to: 375-375
lib/controllers/auth_state_controller.dart (5)
44-46: Avoid late initialization errors for rating fields.
ratingTotalandratingCountare markedlatebut are only assigned whenisUserProfileComplete == true. Accessing them elsewhere risksLateInitializationError.Apply this diff to provide safe defaults:
- late double ratingTotal; - late int ratingCount; + double ratingTotal = 0; + int ratingCount = 0;
160-162: Null/shape-safe numeric conversion for ratings.
userDataDoc.data["ratingTotal"].toDouble()will throw if the value is null or a non-numeric type (e.g., String). Use a safe cast to num before converting.Apply this diff:
- ratingTotal = userDataDoc.data["ratingTotal"].toDouble() ?? 5; - ratingCount = userDataDoc.data["ratingCount"] ?? 1; + ratingTotal = + (userDataDoc.data["ratingTotal"] as num?)?.toDouble() ?? 5.0; + ratingCount = (userDataDoc.data["ratingCount"] as num?)?.toInt() ?? 1;
72-86: Guard against negative index when payload doesn't match any room.If no upcoming room name matches the payload,
indexWherereturns -1, yielding a negativeinitialScrollOffsetand potential UI issues.Apply this diff:
int index = upcomingRoomsController.upcomingRooms .indexWhere((upcomingRoom) => upcomingRoom.name == name); - upcomingRoomsController.upcomingRoomScrollController.value = - ScrollController(initialScrollOffset: UiSizes.height_170 * index); + if (index < 0) { + return; + } + upcomingRoomsController.upcomingRoomScrollController.value = + ScrollController(initialScrollOffset: UiSizes.height_170 * index);
195-232: Make token updates robust: handle null token, de-duplicate, and await updates.
getToken()can return null; current code force-unwraps.- Adds duplicates on every login.
updateDocumentcalls are not awaited, risking lost errors and early return.Apply this refactor:
Future<void> addRegistrationTokentoSubscribedandCreatedUpcomingRooms() async { - final fcmToken = await messaging.getToken(); + final fcmToken = await messaging.getToken(); + if (fcmToken == null || fcmToken.isEmpty) { + log('FCM token is null/empty. Skipping token updates.'); + return; + } //subscribed Upcoming Rooms - List<Document> subscribedUpcomingRooms = await databases.listDocuments( + final List<Document> subscribedUpcomingRooms = await databases.listDocuments( databaseId: upcomingRoomsDatabaseId, collectionId: subscribedUserCollectionId, queries: [ Query.equal("userID", [uid]) ]).then((value) => value.documents); - for (var subscription in subscribedUpcomingRooms) { - List<dynamic> registrationTokens = - subscription.data['registrationTokens']; - registrationTokens.add(fcmToken!); - databases.updateDocument( - databaseId: upcomingRoomsDatabaseId, - collectionId: subscribedUserCollectionId, - documentId: subscription.$id, - data: {"registrationTokens": registrationTokens}); - } + final List<Future<Document>> updates = []; + for (final subscription in subscribedUpcomingRooms) { + final tokens = List<String>.from( + (subscription.data['registrationTokens'] ?? const <String>[])); + if (!tokens.contains(fcmToken)) { + tokens.add(fcmToken); + } + updates.add(databases.updateDocument( + databaseId: upcomingRoomsDatabaseId, + collectionId: subscribedUserCollectionId, + documentId: subscription.$id, + data: {"registrationTokens": tokens})); + } //created Upcoming Rooms - List<Document> createdUpcomingRooms = await databases.listDocuments( + final List<Document> createdUpcomingRooms = await databases.listDocuments( databaseId: upcomingRoomsDatabaseId, collectionId: upcomingRoomsCollectionId, queries: [ Query.equal("creatorUid", [uid]) ]).then((value) => value.documents); - for (var upcomingRoom in createdUpcomingRooms) { - List<dynamic> creatorFcmTokens = upcomingRoom.data['creator_fcm_tokens']; - creatorFcmTokens.add(fcmToken!); - databases.updateDocument( - databaseId: upcomingRoomsDatabaseId, - collectionId: upcomingRoomsCollectionId, - documentId: upcomingRoom.$id, - data: {"creator_fcm_tokens": creatorFcmTokens}); - } + for (final upcomingRoom in createdUpcomingRooms) { + final tokens = List<String>.from( + (upcomingRoom.data['creator_fcm_tokens'] ?? const <String>[])); + if (!tokens.contains(fcmToken)) { + tokens.add(fcmToken); + } + updates.add(databases.updateDocument( + databaseId: upcomingRoomsDatabaseId, + collectionId: upcomingRoomsCollectionId, + documentId: upcomingRoom.$id, + data: {"creator_fcm_tokens": tokens})); + } + await Future.wait(updates); }
234-271: Symmetric fixes for token removal: handle null token, remove all occurrences, and await updates.
- Guard against null token.
- Use
removeWhereto remove all duplicates.- Await
updateDocumentcalls to ensure completion.Apply this refactor:
Future<void> removeRegistrationTokenFromSubscribedUpcomingRooms() async { - final fcmToken = await messaging.getToken(); + final fcmToken = await messaging.getToken(); + if (fcmToken == null || fcmToken.isEmpty) { + log('FCM token is null/empty. Skipping token removals.'); + return; + } //subscribed Upcoming Rooms - List<Document> subscribedUpcomingRooms = await databases.listDocuments( + final List<Document> subscribedUpcomingRooms = await databases.listDocuments( databaseId: upcomingRoomsDatabaseId, collectionId: subscribedUserCollectionId, queries: [ Query.equal("userID", [uid]) ]).then((value) => value.documents); - for (var subscription in subscribedUpcomingRooms) { - List<dynamic> registrationTokens = - subscription.data['registrationTokens']; - registrationTokens.remove(fcmToken!); - databases.updateDocument( - databaseId: upcomingRoomsDatabaseId, - collectionId: subscribedUserCollectionId, - documentId: subscription.$id, - data: {"registrationTokens": registrationTokens}); - } + final List<Future<Document>> updates = []; + for (final subscription in subscribedUpcomingRooms) { + final tokens = List<String>.from( + (subscription.data['registrationTokens'] ?? const <String>[])); + tokens.removeWhere((t) => t == fcmToken); + updates.add(databases.updateDocument( + databaseId: upcomingRoomsDatabaseId, + collectionId: subscribedUserCollectionId, + documentId: subscription.$id, + data: {"registrationTokens": tokens})); + } //created Upcoming Rooms - List<Document> createdUpcomingRooms = await databases.listDocuments( + final List<Document> createdUpcomingRooms = await databases.listDocuments( databaseId: upcomingRoomsDatabaseId, collectionId: upcomingRoomsCollectionId, queries: [ Query.equal("creatorUid", [uid]) ]).then((value) => value.documents); - for (var upcomingRoom in createdUpcomingRooms) { - List<dynamic> creatorFcmTokens = upcomingRoom.data['creator_fcm_tokens']; - creatorFcmTokens.remove(fcmToken!); - databases.updateDocument( - databaseId: upcomingRoomsDatabaseId, - collectionId: upcomingRoomsCollectionId, - documentId: upcomingRoom.$id, - data: {"creator_fcm_tokens": creatorFcmTokens}); - } + for (final upcomingRoom in createdUpcomingRooms) { + final tokens = List<String>.from( + (upcomingRoom.data['creator_fcm_tokens'] ?? const <String>[])); + tokens.removeWhere((t) => t == fcmToken); + updates.add(databases.updateDocument( + databaseId: upcomingRoomsDatabaseId, + collectionId: upcomingRoomsCollectionId, + documentId: upcomingRoom.$id, + data: {"creator_fcm_tokens": tokens})); + } + await Future.wait(updates); }
🧹 Nitpick comments (22)
.github/workflows/run_tests.yml (2)
36-39: Speed up CI by enabling Flutter cache.Enable caching in subosito/flutter-action to reuse SDK and pub cache across runs.
- name: Setup Flutter uses: subosito/flutter-action@v2 with: flutter-version: ${{ env.FLUTTER_VERSION }} + cache: true
10-45: Fix YAML lint issues (trailing spaces and missing EOF newline).YAMLlint flagged trailing spaces (Lines 13, 16, 34) and missing newline at EOF (Line 45). Clean these to avoid CI style checks failing.
- Remove trailing spaces on the flagged lines.
- Ensure the file ends with a single newline.
test/controllers/chapter_player_controller_test.dart (2)
25-28: Assert non-null audioPlayer before dereference for clearer failures.Add an explicit non-null check and use a non-null assertion to make the intent and failures crisper.
- expect(chapterPlayerController.audioPlayer, isA<AudioPlayer>()); - expect(chapterPlayerController.audioPlayer?.releaseMode, ReleaseMode.stop); + expect(chapterPlayerController.audioPlayer, isA<AudioPlayer>()); + expect(chapterPlayerController.audioPlayer, isNotNull); + expect(chapterPlayerController.audioPlayer!.releaseMode, ReleaseMode.stop);
31-42: Consider faking/mocking AudioPlayer to avoid platform-channel dependencies.Using the real AudioPlayer in widget tests can sometimes trigger platform-channel calls. If flakiness appears, replace with a simple fake or mock that satisfies the controller’s needs.
If needed, I can provide a minimal FakeAudioPlayer implementation compatible with the controller API.
lib/controllers/create_room_controller.dart (3)
65-65: Guard Get.back() to avoid unintended route pops.If no dialog is open, Get.back() may pop a route unexpectedly. Guard it.
- Get.back(); + if (Get.isDialogOpen ?? false) { + Get.back(); + }
88-88: Same here: guard Get.back() in error path.Avoid popping routes when no dialog is present.
- Get.back(); + if (Get.isDialogOpen ?? false) { + Get.back(); + }
35-41: Minor: avoid "null" in error messages for invalid/null tags.When tag is null, error displays "... null". Prefer empty string.
- return '${AppLocalizations.of(context)!.invalidTags} $tag'; // Return an error message for invalid tags + return '${AppLocalizations.of(context)!.invalidTags} ${tag ?? ''}'; // Return an error message for invalid tagstest/controllers/create_room_controller_test.dart (1)
6-6: Prefer stable Get import path.Importing internal package paths is brittle. Use the public export.
-import 'package:get/get_navigation/src/root/get_material_app.dart'; +import 'package:get/get.dart';test/controllers/edit_profile_controller_test.dart (2)
55-55: Redundant onInit call.onInit() is already invoked in setUp(). Calling it again in the test is unnecessary and can lead to double-initialization side effects.
- test('check onInit', () { - editProfileController.onInit(); + test('check onInit', () {
24-52: Consider stubbing before onInit if it ever reads from databases.Currently the databases.getDocument stub is set after onInit in setUp. If in future onInit starts hitting databases, tests may become flaky. Prefer registering stubs before invoking onInit.
lib/controllers/explore_story_controller.dart (3)
333-347: Optional: delete likes concurrently for better performance.Deleting each like sequentially can be slow for stories with many likes. Batch with Future.wait.
- for (Document like in storyLikeDocuments) { - await databases.deleteDocument( - databaseId: storyDatabaseId, - collectionId: likeCollectionId, - documentId: like.$id); - } + await Future.wait(storyLikeDocuments.map((like) { + return databases.deleteDocument( + databaseId: storyDatabaseId, + collectionId: likeCollectionId, + documentId: like.$id, + ); + }));
24-24: Typo in property name: openedCategotyStories.Consider renaming to openedCategoryStories for clarity. If used broadly, plan a safe rename across the codebase.
228-233: Typo in parameter name: desciption.Use description for readability and consistency. If public API, coordinate a project-wide rename to avoid breakage.
Also applies to: 264-264
lib/controllers/change_email_controller.dart (1)
110-136: Handle unanticipated Appwrite errors in changeEmailInAuthOnly two error types are handled. For other AppwriteException types, users receive no feedback. Add a default branch to show a generic error.
Apply this diff:
} else if (e.type == generalArgumentInvalid) { customSnackbar( AppLocalizations.of(context)!.tryAgain, AppLocalizations.of(context)!.passwordShort, LogType.error, ); SemanticsService.announce( AppLocalizations.of(context)!.passwordShort, TextDirection.ltr, ); - } + } else { + customSnackbar( + AppLocalizations.of(context)!.tryAgain, + e.toString(), + LogType.error, + ); + SemanticsService.announce( + e.toString(), + TextDirection.ltr, + ); + }test/controllers/edit_profile_controller_test.mocks.dart (1)
13-17: Mockito-generated mocks use implementation imports; pin deps and regenerate on upgradesThe file imports package:appwrite/src/* and package:mockito/src/* (implementation imports). These can break on minor upgrades. Ensure:
- appwrite and mockito versions are pinned in pubspec.yaml.
- You re-run build_runner to regenerate mocks after dependency upgrades.
If the repo prefers not to commit generated code, consider generating in CI. Otherwise, keep this file and regenerate as needed.
Also applies to: 25-26
test/controllers/change_email_controller_test.dart (2)
153-196: Verify both database updates in the end-to-end changeEmail testThe production flow updates both the users and username-email documents. You currently verify only the latter here. Add a verify for the users doc to increase assertion strength (you already verify both in the dedicated changeEmailInDatabases test).
Apply this diff after verifying the username document:
verify(mockDatabases.updateDocument( databaseId: userDatabaseID, collectionId: usernameCollectionID, documentId: 'TestUser', data: { 'email': 'test2@test.com', })).called(1); + verify(mockDatabases.updateDocument( + databaseId: userDatabaseID, + collectionId: usersCollectionID, + documentId: '123', + data: { + 'email': 'test2@test.com', + })).called(1);
106-151: Add negative-path tests (email exists, invalid password)To fully exercise the controller:
- Email already exists -> expect snackbar/announcement and no account.updateEmail call.
- Invalid credentials (userInvalidCredentials) -> expect proper error snackbar and no DB update.
- Short/invalid password (generalArgumentInvalid) -> same.
I can help scaffold these tests with proper stubs if you want.
test/controllers/explore_story_controller_test.dart (2)
174-191: Await the recommendation fetch and simplify loading assertionsWith immediate futures, await the call and assert final state. If you still want to assert the transient loading flag, expose/await an internal future or use FakeAsync, but avoiding real sleeps is preferred.
Apply this diff:
- test('test fetchStoryRecommendation', () async { - exploreStoryController.fetchStoryRecommendation(); - expect(exploreStoryController.isLoadingRecommendedStories.value, true); - await Future.delayed(Duration(seconds: 3)); - expect(exploreStoryController.isLoadingRecommendedStories.value, false); + test('test fetchStoryRecommendation', () async { + await exploreStoryController.fetchStoryRecommendation(); + expect(exploreStoryController.isLoadingRecommendedStories.value, false); expect(exploreStoryController.recommendedStories.length, 2); for (int i = 0; i < exploreStoryController.recommendedStories.length; i++) { expect(mockStoriesList[i].storyId, exploreStoryController.recommendedStories[i].storyId); expect(mockStoriesList[i].title, exploreStoryController.recommendedStories[i].title); expect(mockStoriesList[i].description, exploreStoryController.recommendedStories[i].description); expect(mockStoriesList[i].userIsCreator, exploreStoryController.recommendedStories[i].userIsCreator); //Do not need to check everything as that is handled by the test above } });
127-145: Reduce brittleness of query matchers in stubsMatching the full queries list ([Query.limit(10)] and specific equal queries) can make tests brittle if limits change. Consider using anyNamed('queries') with additional verify() assertions on the queries content, or custom argThat matchers to assert presence of expected filters without coupling to exact lists.
lib/controllers/edit_profile_controller.dart (1)
244-256: Also delete the old profile image file when removing imageCurrently you blank the DB URL but leave the old file in storage, creating an orphan. Delete the file if present.
Apply this diff:
if (removeImage) { imageController.text = ""; // Update user profile picture URL in Database await databases.updateDocument( databaseId: userDatabaseID, collectionId: usersCollectionID, documentId: authStateController.uid!, data: { "profileImageUrl": imageController.text, }, ); + try { + final oldId = authStateController.profileImageID; + if (oldId != null && oldId.isNotEmpty) { + await storage.deleteFile( + bucketId: userProfileImageBucketId, + fileId: oldId, + ); + } + } catch (e) { + log(e.toString()); + } }test/controllers/auth_state_controller_test.dart (1)
36-39: Swap $databaseId and $collectionId to match actual resources.For subscribed-user documents,
$collectionIdshould besubscribedUserCollectionIdand$databaseIdshould beupcomingRoomsDatabaseId. They are currently inverted, which reduces test realism and could mislead future maintenance.Apply this diff for both documents:
- $collectionId: upcomingRoomsCollectionId, - $databaseId: subscribedUserCollectionId, + $collectionId: subscribedUserCollectionId, + $databaseId: upcomingRoomsDatabaseId,Also applies to: 50-53
lib/controllers/auth_state_controller.dart (1)
21-26: Consider making injected fields immutable.
clientandmessagingare assigned once via DI and never reassigned. Making themfinalcommunicates intent and prevents accidental reassignment.Apply this diff:
- Client client; + final Client client; @@ - FirebaseMessaging messaging; + final FirebaseMessaging messaging;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/run_tests.yml(1 hunks).github/workflows/store_deploy_android.yml(1 hunks)android/build/.last_build_id(0 hunks)lib/controllers/auth_state_controller.dart(1 hunks)lib/controllers/change_email_controller.dart(1 hunks)lib/controllers/create_room_controller.dart(1 hunks)lib/controllers/edit_profile_controller.dart(2 hunks)lib/controllers/explore_story_controller.dart(2 hunks)lib/models/resonate_user.freezed.dart(8 hunks)lib/models/resonate_user.g.dart(1 hunks)test/controllers/auth_state_controller_test.dart(1 hunks)test/controllers/auth_state_controller_test.mocks.dart(1 hunks)test/controllers/change_email_controller_test.dart(1 hunks)test/controllers/change_email_controller_test.mocks.dart(1 hunks)test/controllers/chapter_player_controller_test.dart(1 hunks)test/controllers/create_room_controller_test.dart(1 hunks)test/controllers/create_room_controller_test.mocks.dart(1 hunks)test/controllers/edit_profile_controller_test.dart(1 hunks)test/controllers/edit_profile_controller_test.mocks.dart(1 hunks)test/controllers/explore_story_controller_test.dart(1 hunks)test/widget_test.dart(0 hunks)
💤 Files with no reviewable changes (2)
- test/widget_test.dart
- android/build/.last_build_id
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/run_tests.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 45-45: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (14)
.github/workflows/run_tests.yml (1)
21-22: Flutter version 3.32.5 is validThe workflow’s pinned Flutter version (3.32.5) is a valid stable release (June 25, 2025). The current latest stable release is 3.32.8 (July 25, 2025).
• You can continue pinning to 3.32.5 as-is.
• If you need the very latest fixes, update to3.32.8.
• Alternatively, replace the version pin withchannel: stableto automatically use the newest stable Flutter release.
test/controllers/auth_state_controller_test.mocks.dart (1)
1-4: Auto-generated mocks — OK to commit.Looks good. Keep this generated file in sync with dependencies by re-running build_runner when interfaces change.
lib/models/resonate_user.g.dart (1)
18-19: Robust deserialization via _toDouble — LGTM.Centralizing userRating conversion through _toDouble improves resilience to int/string/num inputs.
lib/controllers/create_room_controller.dart (1)
15-15: Good move to constructor-based DI.Switching to a final field and initializing via the constructor improves testability and aligns with DI best practices across the repo.
Also applies to: 23-26
test/controllers/create_room_controller_test.dart (1)
59-62: Test will fail until tag regex is fixed in production code.The case with "Football$" expects an invalid-tag error. With the current implementation using contains(), it will incorrectly pass validation. Apply the CreateRoomController.isValidTag fix (use RegExp.hasMatch) to align behavior with this test.
test/controllers/create_room_controller_test.mocks.dart (1)
1-327: Generated mocks look fine.Mockito-generated file is consistent with ThemeController usage and supports constructor DI in tests. No action required.
lib/controllers/explore_story_controller.dart (1)
17-20: Constructor-based DI is a solid improvement.Final fields with default providers keep previous behavior while enabling injection in tests. Nicely done.
Also applies to: 31-39
lib/controllers/change_email_controller.dart (1)
16-27: Constructor-based DI: solid change and consistent with PR objectivesConstructor injection with sensible Get.find/AppwriteService fallbacks improves testability and removes eager singletons. Aligned with the new testing strategy.
test/controllers/change_email_controller_test.dart (1)
18-35: Good use of constructor DI and comprehensive stubbingThe setup cleanly constructs controllers with mocks, matching the DI refactor in lib/. This unlocks realistic unit/widget tests.
lib/controllers/edit_profile_controller.dart (1)
44-54: Constructor-based DI and moving service initialization out of onInit: good improvementThis aligns the controller with the new testing and DI approach and removes hidden side-effects in onInit.
lib/models/resonate_user.freezed.dart (2)
26-27: LGTM: Defensive JSON conversion for userRating.Annotating
userRatingwith@JsonKey(fromJson: _toDouble)is a safe, backward-compatible way to handle heterogeneous numeric inputs.
353-366: Verify_toDoubleexists and is in scope of resonate_user.dart.This generated code references a private helper
_toDouble. Ensure it is defined inlib/models/resonate_user.dart(the same library/part) so both freezed and json_serializable code can resolve it.If missing, add this in
resonate_user.dart:double? _toDouble(dynamic v) { if (v == null) return null; if (v is num) return v.toDouble(); if (v is String) return double.tryParse(v); return null; }test/controllers/change_email_controller_test.mocks.dart (1)
1-5: Generated mocks look good.Mocks cover Databases, Account, Client, and FirebaseMessaging, enabling isolated controller tests without network calls. No manual changes needed.
lib/controllers/auth_state_controller.dart (1)
88-96: Async work inonInitis not awaited by GetX.
onInitreturnsvoidand is not awaited by the framework; exceptions in the async body may go unnoticed. Consider moving the async initialization to an explicitFuture<void> init()called by the UI entry-point, or ensure robust try/catch and logging around awaited calls withinonInit.Would you like a small refactor that moves async initialization into a dedicated
init()you call after constructing the controller (keeps behavior unchanged for tests)?
| pull_request_target: | ||
| types: [opened] | ||
|
|
There was a problem hiding this comment.
Don’t use pull_request_target to run untrusted PR code; switch to pull_request.
With pull_request_target and the default checkout, you’re testing the base branch, not the PR head. If you switch checkout to the PR head with pull_request_target, you’d run untrusted code with elevated permissions, which is unsafe. Use pull_request so tests run on the PR’s code with the default, safer permission model.
Apply this diff to trigger on PR updates appropriately:
- pull_request_target:
- types: [opened]
+ pull_request:
+ types: [opened, synchronize, reopened]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request_target: | |
| types: [opened] | |
| pull_request: | |
| types: [opened, synchronize, reopened] |
🤖 Prompt for AI Agents
.github/workflows/run_tests.yml around lines 7 to 9: the workflow is using
pull_request_target which runs base-branch code and can execute untrusted PR
code with elevated permissions; change the event to pull_request (e.g., replace
pull_request_target with pull_request) and include appropriate types like
opened, synchronize, reopened so the workflow runs on the PR head and triggers
on PR updates; ensure you do not switch checkout behavior to the PR head under
pull_request_target and keep the default safer permissions.
| permissions: | ||
| pull-requests: write | ||
| contents: read |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce permissions to least privilege.
Tests don’t need write access to PRs. Remove pull-requests: write to avoid unnecessary permissions.
-permissions:
- pull-requests: write
- contents: read
+permissions:
+ contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| pull-requests: write | |
| contents: read | |
| permissions: | |
| contents: read |
🤖 Prompt for AI Agents
.github/workflows/run_tests.yml around lines 10 to 12: the workflow currently
grants pull-requests: write which is excessive for running tests; change the
permissions block to remove or downgrade pull-requests write access (e.g.,
delete the pull-requests entry or set pull-requests: read if needed) so the
workflow uses least privilege while keeping contents: read.
| - name: 📉 Run all app tests | ||
| run: flutter test |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run tests before decoding secrets and building (fail-fast, minimize secret exposure).
Move the “Run all app tests” step to immediately after dependency install and before any secret decoding or signing steps. This reduces risk of leaking secrets via logs on failing tests and saves CI minutes by failing earlier.
Apply this diff here to remove the test step from its current position:
- - name: 📉 Run all app tests
- run: flutter testThen add it right after “📦 Install dependencies” (outside the selected range), for example:
# Place this after "📦 Install dependencies" and before any "Decode ..." steps
- name: 📉 Run all app tests
run: flutter testOptional: enable caching for faster runs (outside the selected range):
- name: Setup Flutter
uses: subosito/flutter-action@v2
with:
flutter-version: ${{ env.FLUTTER_VERSION }}
cache: true🤖 Prompt for AI Agents
.github/workflows/store_deploy_android.yml around lines 71-72: the "📉 Run all
app tests" step is currently placed after secret decoding and signing steps;
remove that step from lines 71-72 and re-insert it immediately after the "📦
Install dependencies" step (before any "Decode ..." or signing steps) so tests
run fail-fast without exposing secrets; keep the same step name and command
("flutter test"); optionally enable Flutter action caching by setting cache:
true on the Setup Flutter action (outside this range).
| final emailController = TextEditingController(); | ||
| final passwordController = TextEditingController(); | ||
| ChangeEmailController({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dispose TextEditingControllers to avoid leaks
emailController and passwordController are never disposed. Dispose them in onClose().
Apply this diff near the end of the class:
}
}
+
+ @override
+ void onClose() {
+ emailController.dispose();
+ passwordController.dispose();
+ super.onClose();
+ }Also applies to: 199-201
🤖 Prompt for AI Agents
In lib/controllers/change_email_controller.dart around lines 18-20 (and also
applies to lines 199-201), the TextEditingController instances emailController
and passwordController are created but never disposed; add an override of
onClose() in the ChangeEmailController that calls emailController.dispose() and
passwordController.dispose(), then calls super.onClose(), to properly release
resources and avoid memory leaks.
| databaseId: upcomingRoomsDatabaseId, | ||
| collectionId: upcomingRoomsCollectionId, | ||
| queries: [Query.equal("creatorUid", "123")])) | ||
| .thenAnswer((_) => Future.value( | ||
| DocumentList(total: 2, documents: mockUpcomingRoomsDocuments))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stub mismatch: Query.equal value type should match production (use a list).
Production calls Query.equal("creatorUid", [uid]), but the stub uses a string. This prevents the when(...listDocuments...) from matching, causing the test to hit the real method or return null.
Apply this diff to align the stub with production:
- when(mockDatabases.listDocuments(
- databaseId: upcomingRoomsDatabaseId,
- collectionId: upcomingRoomsCollectionId,
- queries: [Query.equal("creatorUid", "123")]))
+ when(mockDatabases.listDocuments(
+ databaseId: upcomingRoomsDatabaseId,
+ collectionId: upcomingRoomsCollectionId,
+ queries: [Query.equal("creatorUid", ["123"])]))
.thenAnswer((_) => Future.value(
DocumentList(total: 2, documents: mockUpcomingRoomsDocuments)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| queries: [Query.equal("creatorUid", "123")])) | |
| .thenAnswer((_) => Future.value( | |
| DocumentList(total: 2, documents: mockUpcomingRoomsDocuments))); | |
| when(mockDatabases.listDocuments( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| queries: [Query.equal("creatorUid", ["123"])])) | |
| .thenAnswer((_) => Future.value( | |
| DocumentList(total: 2, documents: mockUpcomingRoomsDocuments))); |
🤖 Prompt for AI Agents
In test/controllers/auth_state_controller_test.dart around lines 156 to 160, the
stub for listDocuments uses Query.equal("creatorUid", "123") but production uses
a list value; update the stub to use a list (e.g. Query.equal("creatorUid",
["123"])) so the argument matcher matches the real call and the
when(...listDocuments...) returns the intended DocumentList; adjust any other
similar stubs in this test to use list values for Query.equal where production
passes a list.
| test('test removeRegistrationTokenFromSubscribedUpcomingRooms', () async { | ||
| await authStateController.setUserProfileData(); | ||
| await authStateController | ||
| .removeRegistrationTokenFromSubscribedUpcomingRooms(); | ||
| verify(mockDatabases.updateDocument( | ||
| databaseId: upcomingRoomsDatabaseId, | ||
| collectionId: subscribedUserCollectionId, | ||
| documentId: 'subUserDoc1', | ||
| data: { | ||
| 'registrationTokens': ['token1', 'token2', 'mockToken'] | ||
| }, | ||
| )).called(1); | ||
| verify(mockDatabases.updateDocument( | ||
| databaseId: upcomingRoomsDatabaseId, | ||
| collectionId: subscribedUserCollectionId, | ||
| documentId: 'subUserDoc2', | ||
| data: { | ||
| 'registrationTokens': ['token1', 'token2', 'mockToken'] | ||
| }, | ||
| )).called(1); | ||
| verify(mockDatabases.updateDocument( | ||
| databaseId: upcomingRoomsDatabaseId, | ||
| collectionId: upcomingRoomsCollectionId, | ||
| documentId: 'room1', | ||
| data: { | ||
| 'creator_fcm_tokens': ['token1', 'token2', 'mockToken'] | ||
| }, | ||
| )).called(1); | ||
| verify(mockDatabases.updateDocument( | ||
| databaseId: upcomingRoomsDatabaseId, | ||
| collectionId: upcomingRoomsCollectionId, | ||
| documentId: 'room2', | ||
| data: { | ||
| 'creator_fcm_tokens': ['token1', 'token2', 'mockToken'] | ||
| }, | ||
| )).called(1); | ||
| }); |
There was a problem hiding this comment.
Fix expectations: removal should not keep the removed token.
removeRegistrationTokenFromSubscribedUpcomingRooms removes the FCM token. Initial lists contain a single 'mockToken', so after removal, expected lists must not include 'mockToken'.
Apply this diff to correct expectations:
data: {
- 'registrationTokens': ['token1', 'token2', 'mockToken']
+ 'registrationTokens': ['token1', 'token2']
},
@@
data: {
- 'registrationTokens': ['token1', 'token2', 'mockToken']
+ 'registrationTokens': ['token1', 'token2']
},
@@
data: {
- 'creator_fcm_tokens': ['token1', 'token2', 'mockToken']
+ 'creator_fcm_tokens': ['token1', 'token2']
},
@@
data: {
- 'creator_fcm_tokens': ['token1', 'token2', 'mockToken']
+ 'creator_fcm_tokens': ['token1', 'token2']
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('test removeRegistrationTokenFromSubscribedUpcomingRooms', () async { | |
| await authStateController.setUserProfileData(); | |
| await authStateController | |
| .removeRegistrationTokenFromSubscribedUpcomingRooms(); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: subscribedUserCollectionId, | |
| documentId: 'subUserDoc1', | |
| data: { | |
| 'registrationTokens': ['token1', 'token2', 'mockToken'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: subscribedUserCollectionId, | |
| documentId: 'subUserDoc2', | |
| data: { | |
| 'registrationTokens': ['token1', 'token2', 'mockToken'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| documentId: 'room1', | |
| data: { | |
| 'creator_fcm_tokens': ['token1', 'token2', 'mockToken'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| documentId: 'room2', | |
| data: { | |
| 'creator_fcm_tokens': ['token1', 'token2', 'mockToken'] | |
| }, | |
| )).called(1); | |
| }); | |
| test('test removeRegistrationTokenFromSubscribedUpcomingRooms', () async { | |
| await authStateController.setUserProfileData(); | |
| await authStateController | |
| .removeRegistrationTokenFromSubscribedUpcomingRooms(); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: subscribedUserCollectionId, | |
| documentId: 'subUserDoc1', | |
| data: { | |
| 'registrationTokens': ['token1', 'token2'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: subscribedUserCollectionId, | |
| documentId: 'subUserDoc2', | |
| data: { | |
| 'registrationTokens': ['token1', 'token2'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| documentId: 'room1', | |
| data: { | |
| 'creator_fcm_tokens': ['token1', 'token2'] | |
| }, | |
| )).called(1); | |
| verify(mockDatabases.updateDocument( | |
| databaseId: upcomingRoomsDatabaseId, | |
| collectionId: upcomingRoomsCollectionId, | |
| documentId: 'room2', | |
| data: { | |
| 'creator_fcm_tokens': ['token1', 'token2'] | |
| }, | |
| )).called(1); | |
| }); |
🤖 Prompt for AI Agents
In test/controllers/auth_state_controller_test.dart around lines 249 to 285, the
verifications currently expect the removed FCM token 'mockToken' to remain in
the arrays; update the expected data in all four verify(...) calls so the
registration/creator token lists do NOT include 'mockToken' (i.e., use
['token1', 'token2'] for each updateDocument verification) so the assertions
match the removal behavior.
| test('test signup', () async { | ||
| authStateController.signup('test@test.com', 'anyPassword'); | ||
| expect(authStateController.email, 'test@test.com'); | ||
| }); |
There was a problem hiding this comment.
Await the async signup to avoid flakiness.
The test callback is async but doesn't await signup(...). The assertion may run before the controller updates state.
Apply this diff:
- authStateController.signup('test@test.com', 'anyPassword');
+ await authStateController.signup('test@test.com', 'anyPassword');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('test signup', () async { | |
| authStateController.signup('test@test.com', 'anyPassword'); | |
| expect(authStateController.email, 'test@test.com'); | |
| }); | |
| test('test signup', () async { | |
| - authStateController.signup('test@test.com', 'anyPassword'); | |
| + await authStateController.signup('test@test.com', 'anyPassword'); | |
| expect(authStateController.email, 'test@test.com'); | |
| }); |
🤖 Prompt for AI Agents
In test/controllers/auth_state_controller_test.dart around lines 287 to 290, the
async test calls authStateController.signup(...) but does not await it, causing
the expect to run before state updates; update the test to await the async
signup call (await authStateController.signup('test@test.com', 'anyPassword');)
so the controller has time to update before asserting authStateController.email
equals 'test@test.com'.
| void main() { | ||
| ChapterPlayerController chapterPlayerController = ChapterPlayerController(); | ||
| test('check initial values', () { | ||
| expect(chapterPlayerController.currentPage.value, 0.0); | ||
| expect(chapterPlayerController.lyricProgress.value, 0.0); | ||
| expect(chapterPlayerController.sliderProgress.value, 0.0); | ||
| expect(chapterPlayerController.isPlaying.value, false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shared state across tests; instantiate controller per test and dispose.
A single top-level ChapterPlayerController instance leaks state between tests and may retain resources (e.g., AudioPlayer). Initialize in setUp and dispose in tearDown to prevent flakiness and resource leaks.
void main() {
- ChapterPlayerController chapterPlayerController = ChapterPlayerController();
+ late ChapterPlayerController chapterPlayerController;
+
+ setUp(() {
+ chapterPlayerController = ChapterPlayerController();
+ });
+
+ tearDown(() async {
+ // Dispose audio resources to avoid leaks across tests
+ await chapterPlayerController.audioPlayer?.dispose();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void main() { | |
| ChapterPlayerController chapterPlayerController = ChapterPlayerController(); | |
| test('check initial values', () { | |
| expect(chapterPlayerController.currentPage.value, 0.0); | |
| expect(chapterPlayerController.lyricProgress.value, 0.0); | |
| expect(chapterPlayerController.sliderProgress.value, 0.0); | |
| expect(chapterPlayerController.isPlaying.value, false); | |
| }); | |
| void main() { | |
| late ChapterPlayerController chapterPlayerController; | |
| setUp(() { | |
| chapterPlayerController = ChapterPlayerController(); | |
| }); | |
| tearDown(() async { | |
| // Dispose audio resources to avoid leaks across tests | |
| await chapterPlayerController.audioPlayer?.dispose(); | |
| }); | |
| test('check initial values', () { | |
| expect(chapterPlayerController.currentPage.value, 0.0); | |
| expect(chapterPlayerController.lyricProgress.value, 0.0); | |
| expect(chapterPlayerController.sliderProgress.value, 0.0); | |
| expect(chapterPlayerController.isPlaying.value, false); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In test/controllers/chapter_player_controller_test.dart around lines 8 to 15,
the test uses a single top-level ChapterPlayerController instance which can leak
state and resources between tests; change to create a new
ChapterPlayerController in setUp() before each test and assign it to a local
variable, and call the controller's dispose/cleanup method (or explicitly close
its AudioPlayer) in tearDown() after each test so no shared state or open
resources remain. Ensure tests reference the setup instance rather than a
file‑scope instance.
| when(databases.listDocuments( | ||
| databaseId: storyDatabaseId, | ||
| collectionId: storyCollectionId, | ||
| queries: [ | ||
| Query.equal( | ||
| 'creatorId', exploreStoryController.authStateController.uid) | ||
| ])).thenAnswer((_) => Future.delayed( | ||
| Duration(seconds: 2), | ||
| () => DocumentList(total: 1, documents: [mockStoryDocuments[1]]), | ||
| )); | ||
| when(databases | ||
| .listDocuments( | ||
| databaseId: storyDatabaseId, | ||
| collectionId: storyCollectionId, | ||
| queries: [Query.limit(10)])).thenAnswer((_) => Future.delayed( | ||
| Duration(seconds: 2), | ||
| () => DocumentList(total: 2, documents: mockStoryDocuments), | ||
| )); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid real multi-second delays in tests; return immediate futures
The 2-second delays slow the suite and CI. Return immediate futures instead. The loading-state test can await the method and assert before/after states.
Apply this diff to stubs:
- when(databases.listDocuments(
+ when(databases.listDocuments(
databaseId: storyDatabaseId,
collectionId: storyCollectionId,
queries: [
Query.equal(
'creatorId', exploreStoryController.authStateController.uid)
- ])).thenAnswer((_) => Future.delayed(
- Duration(seconds: 2),
- () => DocumentList(total: 1, documents: [mockStoryDocuments[1]]),
- ));
- when(databases
- .listDocuments(
+ ])).thenAnswer((_) async =>
+ DocumentList(total: 1, documents: [mockStoryDocuments[1]]));
+ when(databases.listDocuments(
databaseId: storyDatabaseId,
collectionId: storyCollectionId,
- queries: [Query.limit(10)])).thenAnswer((_) => Future.delayed(
- Duration(seconds: 2),
- () => DocumentList(total: 2, documents: mockStoryDocuments),
- ));
+ queries: [Query.limit(10)])).thenAnswer((_) async =>
+ DocumentList(total: 2, documents: mockStoryDocuments));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| when(databases.listDocuments( | |
| databaseId: storyDatabaseId, | |
| collectionId: storyCollectionId, | |
| queries: [ | |
| Query.equal( | |
| 'creatorId', exploreStoryController.authStateController.uid) | |
| ])).thenAnswer((_) => Future.delayed( | |
| Duration(seconds: 2), | |
| () => DocumentList(total: 1, documents: [mockStoryDocuments[1]]), | |
| )); | |
| when(databases | |
| .listDocuments( | |
| databaseId: storyDatabaseId, | |
| collectionId: storyCollectionId, | |
| queries: [Query.limit(10)])).thenAnswer((_) => Future.delayed( | |
| Duration(seconds: 2), | |
| () => DocumentList(total: 2, documents: mockStoryDocuments), | |
| )); | |
| }); | |
| when(databases.listDocuments( | |
| databaseId: storyDatabaseId, | |
| collectionId: storyCollectionId, | |
| queries: [ | |
| Query.equal( | |
| 'creatorId', exploreStoryController.authStateController.uid) | |
| ])).thenAnswer((_) async => | |
| DocumentList(total: 1, documents: [mockStoryDocuments[1]])); | |
| when(databases.listDocuments( | |
| databaseId: storyDatabaseId, | |
| collectionId: storyCollectionId, | |
| queries: [Query.limit(10)])).thenAnswer((_) async => | |
| DocumentList(total: 2, documents: mockStoryDocuments)); | |
| }); |
🤖 Prompt for AI Agents
In test/controllers/explore_story_controller_test.dart around lines 127 to 145,
the mocks use Future.delayed with 2-second waits which slow tests; change them
to return immediate futures instead (e.g., replace Future.delayed(..., () =>
DocumentList(...)) with Future.value(DocumentList(...)) or an async thenAnswer
that immediately returns the DocumentList) so the tests run fast while the
loading-state test can still await the call and assert before/after states.
|
✅ PR Closed - Thank You, @M4dhav!
We appreciate your effort and look forward to more contributions from you! 🤝 |
1 participant
Description
This PR adds tests for ~50% of key app functionalities that can be efficiently tested using a combination of widget and unit tests. It also modifies controllers and adds constructors for them so that they can be mocked for said tests. Additionally, a workflow to automatically run said tests on all PRs is also added.
Fixes #508
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
All Tests were run using flutter test and all passed.
Checklist:
Maintainer Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores