test(auth): add unit testing suite for LocalStorageService#541
test(auth): add unit testing suite for LocalStorageService#541may-tas wants to merge 1 commit intoCircuitVerse:masterfrom
Conversation
✅ Deploy Preview for cv-mobile-app-web ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis pull request adds explicit null-handling behavior to 🚥 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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/local_storage_service.dart (1)
82-84:⚠️ Potential issue | 🟠 MajorBug:
currentUser = nulldoes not trigger key removal.When
userToSaveis null,json.encode(userToSave?.toJson())evaluates tojson.encode(null)which returns the string"null"— notnull. This bypasses the new null-handling logic at line 34 and stores the literal string"null"instead of deleting the key.The getter's workaround (
|| userJson == 'null'at line 75) masks this bug, but the behavior is inconsistent with thetokensetter and the PR objective of properly removing keys on null assignment.🐛 Proposed fix to properly delete the key when userToSave is null
set currentUser(User? userToSave) { - _saveToDisk(USER, json.encode(userToSave?.toJson())); + _saveToDisk(USER, userToSave == null ? null : json.encode(userToSave.toJson())); }
🧹 Nitpick comments (2)
test/service_tests/local_storage_service_test.dart (2)
11-19: Consider reinitializing_preferencesinsetUpfor proper test isolation.The
storageinstance is obtained once insetUpAll, butLocalStorageService._preferencesis a static variable. WhileSharedPreferences.setMockInitialValues({})insetUpreplaces the underlying mock store, this pattern is fragile—if the mock implementation changes or the service caches values differently, tests could leak state.For robust isolation, consider resetting the static
_preferencestonullintearDown(would require exposing a test helper or@visibleForTestingreset method), or re-fetching the storage instance after each mock reset.
42-66: Test passes but doesn't verify actual key deletion.This test will pass even with the bug in the
currentUsersetter (which stores"null"string instead of removing the key). The getter's workaround masks the issue.Once the setter bug is fixed, consider adding a verification that the key is actually removed from SharedPreferences:
♻️ Optional: Add explicit key-removal verification
// After setting currentUser = null, verify key is removed storage.currentUser = null; expect(storage.currentUser, null); // Optional: verify key is actually removed from SharedPreferences final prefs = await SharedPreferences.getInstance(); expect(prefs.containsKey('logged_in_user'), false);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb061e09-9b5a-470e-a2ce-11879ee571ad
📒 Files selected for processing (2)
lib/services/local_storage_service.darttest/service_tests/local_storage_service_test.dart
Fixes #540
Describe the changes you have made in this PR -
test/service_tests/local_storage_service_test.dartfor the coreLocalStorageService.database_service_test.dart, primarily utilizingSharedPreferences.setMockInitialValues({}).token,isLoggedIn, andcurrentUserserialize, retrieve, and delete values reliably.Screenshots of the changes (If any) -
N/A (Test Suite only)
Summary by CodeRabbit
Bug Fixes
Tests