Add comprehensive unit tests for AuthProvider#1034
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces a comprehensive Flutter unit test suite for ChangesAuthProvider Unit Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Signed-off-by: andoriyaprashant <prashantandoriya@gmail.com>
![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
🧹 Nitpick comments (2)
test/auth_provider_test.dart (2)
7-9: ⚡ Quick winAdd
tearDown(() => authProvider.dispose())to avoid resource leaks.
AuthProviderextendsChangeNotifier. Without disposing it after each test, registered listeners are never freed and the object is left in an indeterminate state for the GC. Add atearDownblock alongside the existingsetUp:♻️ Proposed fix
setUp(() { authProvider = AuthProvider(); }); + tearDown(() { + authProvider.dispose(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/auth_provider_test.dart` around lines 7 - 9, Add a tearDown that calls authProvider.dispose() to prevent leaking listeners from AuthProvider (which extends ChangeNotifier); alongside the existing setUp that instantiates authProvider = AuthProvider(), add a tearDown(() => authProvider.dispose()) so each test cleans up the AuthProvider instance and frees registered listeners.
211-244: ⚡ Quick win
clearUsertests do not assert string field behavior.The group only verifies boolean flags. If
clearUser()is expected to also resetuserId,username,userEmail,loginError, orpacketError, regressions in those resets would go undetected. Consider adding assertions (or an explicit "does not reset" assertion) for all string-typed fields thatclearUser()could touch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/auth_provider_test.dart` around lines 211 - 244, Update the clearUser test group to also assert string-typed fields are handled correctly by clearUser: after calling authProvider.clearUser() add assertions that authProvider.userId, authProvider.username, authProvider.userEmail, authProvider.loginError, and authProvider.packetError are reset to their expected cleared values (e.g., empty string or null per implementation); if any of these string fields are intended to be preserved instead, add explicit "does not reset" assertions for those fields; locate these checks alongside the existing boolean expectations in the 'clearUser Tests' group that call authProvider.clearUser().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/auth_provider_test.dart`:
- Around line 204-208: The setter setIsLoggingIn currently ignores its parameter
and always sets _isLoggingIn to false; change setIsLoggingIn(bool value) to
assign _isLoggingIn = value so the flag reflects the passed value, and update
the test 'setIsLoggingIn follows current implementation' to assert
authProvider.isLoggingIn is true after calling authProvider.setIsLoggingIn(true)
so the test matches the corrected behavior.
---
Nitpick comments:
In `@test/auth_provider_test.dart`:
- Around line 7-9: Add a tearDown that calls authProvider.dispose() to prevent
leaking listeners from AuthProvider (which extends ChangeNotifier); alongside
the existing setUp that instantiates authProvider = AuthProvider(), add a
tearDown(() => authProvider.dispose()) so each test cleans up the AuthProvider
instance and frees registered listeners.
- Around line 211-244: Update the clearUser test group to also assert
string-typed fields are handled correctly by clearUser: after calling
authProvider.clearUser() add assertions that authProvider.userId,
authProvider.username, authProvider.userEmail, authProvider.loginError, and
authProvider.packetError are reset to their expected cleared values (e.g., empty
string or null per implementation); if any of these string fields are intended
to be preserved instead, add explicit "does not reset" assertions for those
fields; locate these checks alongside the existing boolean expectations in the
'clearUser Tests' group that call authProvider.clearUser().
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9a76132-9bd8-487e-a5bf-7989d5289363
📒 Files selected for processing (1)
test/auth_provider_test.dart
| test('setIsLoggingIn follows current implementation', () { | ||
| authProvider.setIsLoggingIn(true); | ||
|
|
||
| expect(authProvider.isLoggingIn, false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the AuthProvider implementation and inspect setIsLoggingIn
fd -e dart auth_provider --exec grep -n "setIsLoggingIn\|isLoggingIn" {}Repository: mosip/android-registration-client
Length of output: 547
Fix setIsLoggingIn implementation — it ignores the parameter and always sets the flag to false.
The implementation (line 113-114) is:
setIsLoggingIn(bool value) {
_isLoggingIn = false;
}
This is a bug. The setter discards the value parameter and unconditionally sets _isLoggingIn = false, breaking the setter's purpose. Update it to:
setIsLoggingIn(bool value) {
_isLoggingIn = value;
}
The test (line 204-208) must also be corrected to assert the intended behavior:
test('setIsLoggingIn follows current implementation', () {
authProvider.setIsLoggingIn(true);
expect(authProvider.isLoggingIn, true); // Should be true, not false
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/auth_provider_test.dart` around lines 204 - 208, The setter
setIsLoggingIn currently ignores its parameter and always sets _isLoggingIn to
false; change setIsLoggingIn(bool value) to assign _isLoggingIn = value so the
flag reflects the passed value, and update the test 'setIsLoggingIn follows
current implementation' to assert authProvider.isLoggingIn is true after calling
authProvider.setIsLoggingIn(true) so the test matches the corrected behavior.
|
Hi @SachinPremkumar @Pragya279 could you please review this Pull request and let me know if any changes are needed |
1 participant
Fixes #1033
Summary
This Pull Request adds comprehensive unit tests for
AuthProvidercovering its internal state management and listener behavior.Added Test Coverage
clearUser()functionalityResult
Summary by CodeRabbit
Release Notes
Note: This release includes internal testing improvements with no user-facing changes.