🧪 [testing improvement] Untested Exception Path in firebase_utils.reference#135
Closed
DaTiC0 wants to merge 8 commits into
Closed
🧪 [testing improvement] Untested Exception Path in firebase_utils.reference#135DaTiC0 wants to merge 8 commits into
DaTiC0 wants to merge 8 commits into
Conversation
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
Reviewer's GuideAdds a comprehensive unittest suite for firebase_utils, covering the firebase reference behavior (including exception paths), user scope normalization, mock references, and user device state reference helpers using firebase_admin mocks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite tests/test_firebase_utils.py to verify the functionality of Firebase utility methods, including mock reference behaviors, normalization, and exception handling. The review feedback highlights a potential issue with test pollution due to global modifications of sys.modules at the module level. It is recommended to save and restore the original state of sys.modules using a tearDownModule function to ensure proper test isolation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+6
to
+17
| # Setup mocks for firebase_admin before importing firebase_utils | ||
| mock_db = MagicMock() | ||
| mock_firebase_admin = MagicMock() | ||
| mock_firebase_admin.db = mock_db | ||
| sys.modules['firebase_admin'] = mock_firebase_admin | ||
| sys.modules['firebase_admin.db'] = mock_db | ||
|
|
||
| # Clear firebase_utils from sys.modules to ensure a fresh import with our mocks | ||
| if 'firebase_utils' in sys.modules: | ||
| del sys.modules['firebase_utils'] | ||
|
|
||
| import firebase_utils |
Contributor
There was a problem hiding this comment.
Modifying sys.modules globally at the module level without restoring it can lead to test pollution, where other test files in the suite are affected by these mocks. This can cause flaky tests or unexpected failures depending on the order in which tests are executed.
To ensure proper test isolation, we should save the original state of sys.modules and restore it in a tearDownModule function.
# Save original modules to prevent test pollution
_original_modules = {
name: sys.modules.get(name)
for name in ['firebase_admin', 'firebase_admin.db', 'firebase_utils']
}
# Setup mocks for firebase_admin before importing firebase_utils
mock_db = MagicMock()
mock_firebase_admin = MagicMock()
mock_firebase_admin.db = mock_db
sys.modules['firebase_admin'] = mock_firebase_admin
sys.modules['firebase_admin.db'] = mock_db
# Clear firebase_utils from sys.modules to ensure a fresh import with our mocks
if 'firebase_utils' in sys.modules:
del sys.modules['firebase_utils']
import firebase_utils
def tearDownModule():
# Restore original sys.modules state to prevent test pollution
for name, module in _original_modules.items():
if module is None:
sys.modules.pop(name, None)
else:
sys.modules[name] = module![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The manual manipulation of
sys.modulesto injectfirebase_adminandfirebase_admin.dbcould leak into other tests; consider capturing and restoring the originalsys.modulesentries in a module-level setup/teardown or context to avoid cross-test pollution. - The
if __name__ == '__main__': unittest.main()block is not needed when the test suite is run by the project's test runner and can be removed to keep the test file focused on its test definitions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual manipulation of `sys.modules` to inject `firebase_admin` and `firebase_admin.db` could leak into other tests; consider capturing and restoring the original `sys.modules` entries in a module-level setup/teardown or context to avoid cross-test pollution.
- The `if __name__ == '__main__': unittest.main()` block is not needed when the test suite is run by the project's test runner and can be removed to keep the test file focused on its test definitions.
## Individual Comments
### Comment 1
<location path="tests/test_firebase_utils.py" line_range="64-75" />
<code_context>
+ self.assertIsInstance(ref, firebase_utils.MockRef)
+ mock_db.reference.assert_not_called()
+
+ def test_normalize_user_scope(self):
+ self.assertEqual(firebase_utils._normalize_user_scope("user123"), "user123")
+ self.assertEqual(firebase_utils._normalize_user_scope(123), "123")
+ self.assertEqual(firebase_utils._normalize_user_scope(" user123 "), "user123")
+ self.assertIsNone(firebase_utils._normalize_user_scope(None))
+ self.assertIsNone(firebase_utils._normalize_user_scope(""))
+ self.assertIsNone(firebase_utils._normalize_user_scope(" "))
+ self.assertIsNone(firebase_utils._normalize_user_scope("user/123"))
+ self.assertIsNone(firebase_utils._normalize_user_scope("user\\123"))
+ self.assertIsNone(firebase_utils._normalize_user_scope("user..123"))
+
+ def test_get_user_device_states_ref_valid(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add integration tests that exercise `_normalize_user_scope` via `reference()` to prove the normalized value is actually used in paths.
Currently `_normalize_user_scope` is only tested in isolation. Please add an integration-style test for `reference(user_id=...)` that asserts the Firebase path uses the normalized value, e.g.:
- `reference(user_id=" user123 ")` calls `mock_db.reference("/users/user123/devices")`
- `reference(user_id=123)` calls `mock_db.reference("/users/123/devices")`
This guards against regressions where `reference` stops using `_normalize_user_scope` correctly.
```suggestion
def test_normalize_user_scope(self):
self.assertEqual(firebase_utils._normalize_user_scope("user123"), "user123")
self.assertEqual(firebase_utils._normalize_user_scope(123), "123")
self.assertEqual(firebase_utils._normalize_user_scope(" user123 "), "user123")
self.assertIsNone(firebase_utils._normalize_user_scope(None))
self.assertIsNone(firebase_utils._normalize_user_scope(""))
self.assertIsNone(firebase_utils._normalize_user_scope(" "))
self.assertIsNone(firebase_utils._normalize_user_scope("user/123"))
self.assertIsNone(firebase_utils._normalize_user_scope("user\\123"))
self.assertIsNone(firebase_utils._normalize_user_scope("user..123"))
def test_reference_uses_normalized_user_scope_str_input(self):
with patch('firebase_utils.FIREBASE_AVAILABLE', True):
# Ensure reference() routes to the real db mock and uses the normalized user_id in the path
firebase_utils.db = mock_db
mock_db.reference.reset_mock()
firebase_utils.reference(user_id=" user123 ")
mock_db.reference.assert_called_once_with("/users/user123/devices")
def test_reference_uses_normalized_user_scope_int_input(self):
with patch('firebase_utils.FIREBASE_AVAILABLE', True):
# Ensure integer user_id is normalized to its string representation in the path
firebase_utils.db = mock_db
mock_db.reference.reset_mock()
firebase_utils.reference(user_id=123)
mock_db.reference.assert_called_once_with("/users/123/devices")
def test_get_user_device_states_ref_valid(self):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+64
to
+75
| def test_normalize_user_scope(self): | ||
| self.assertEqual(firebase_utils._normalize_user_scope("user123"), "user123") | ||
| self.assertEqual(firebase_utils._normalize_user_scope(123), "123") | ||
| self.assertEqual(firebase_utils._normalize_user_scope(" user123 "), "user123") | ||
| self.assertIsNone(firebase_utils._normalize_user_scope(None)) | ||
| self.assertIsNone(firebase_utils._normalize_user_scope("")) | ||
| self.assertIsNone(firebase_utils._normalize_user_scope(" ")) | ||
| self.assertIsNone(firebase_utils._normalize_user_scope("user/123")) | ||
| self.assertIsNone(firebase_utils._normalize_user_scope("user\\123")) | ||
| self.assertIsNone(firebase_utils._normalize_user_scope("user..123")) | ||
|
|
||
| def test_get_user_device_states_ref_valid(self): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add integration tests that exercise _normalize_user_scope via reference() to prove the normalized value is actually used in paths.
Currently _normalize_user_scope is only tested in isolation. Please add an integration-style test for reference(user_id=...) that asserts the Firebase path uses the normalized value, e.g.:
reference(user_id=" user123 ")callsmock_db.reference("/users/user123/devices")reference(user_id=123)callsmock_db.reference("/users/123/devices")
This guards against regressions wherereferencestops using_normalize_user_scopecorrectly.
Suggested change
| def test_normalize_user_scope(self): | |
| self.assertEqual(firebase_utils._normalize_user_scope("user123"), "user123") | |
| self.assertEqual(firebase_utils._normalize_user_scope(123), "123") | |
| self.assertEqual(firebase_utils._normalize_user_scope(" user123 "), "user123") | |
| self.assertIsNone(firebase_utils._normalize_user_scope(None)) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope(" ")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user/123")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user\\123")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user..123")) | |
| def test_get_user_device_states_ref_valid(self): | |
| def test_normalize_user_scope(self): | |
| self.assertEqual(firebase_utils._normalize_user_scope("user123"), "user123") | |
| self.assertEqual(firebase_utils._normalize_user_scope(123), "123") | |
| self.assertEqual(firebase_utils._normalize_user_scope(" user123 "), "user123") | |
| self.assertIsNone(firebase_utils._normalize_user_scope(None)) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope(" ")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user/123")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user\\123")) | |
| self.assertIsNone(firebase_utils._normalize_user_scope("user..123")) | |
| def test_reference_uses_normalized_user_scope_str_input(self): | |
| with patch('firebase_utils.FIREBASE_AVAILABLE', True): | |
| # Ensure reference() routes to the real db mock and uses the normalized user_id in the path | |
| firebase_utils.db = mock_db | |
| mock_db.reference.reset_mock() | |
| firebase_utils.reference(user_id=" user123 ") | |
| mock_db.reference.assert_called_once_with("/users/user123/devices") | |
| def test_reference_uses_normalized_user_scope_int_input(self): | |
| with patch('firebase_utils.FIREBASE_AVAILABLE', True): | |
| # Ensure integer user_id is normalized to its string representation in the path | |
| firebase_utils.db = mock_db | |
| mock_db.reference.reset_mock() | |
| firebase_utils.reference(user_id=123) | |
| mock_db.reference.assert_called_once_with("/users/123/devices") | |
| def test_get_user_device_states_ref_valid(self): |
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes. - Removed unused logging import in test file. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes. - Removed unused logging import in test file. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes. - Removed unused logging import in test file. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes. - Removed unused logging import in test file. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Improved test isolation for exception path to ensure it passes in CI environment. - Fixed lint issues in test files. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes.
…erence - Added comprehensive tests for firebase_utils.py. - Verified exception handling path in reference() function. - Improved test isolation for exception path to ensure it passes in CI environment. - Fixed lint issues in test files. - Tested user scope normalization and device state reference building. - Added tests for MockRef and MockChild classes.
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.
🎯 What: The testing gap addressed (Exception path in
firebase_utils.reference).📊 Coverage: Scenarios now tested (Firebase availability, exception handling, user scope normalization, mock data implementation).
✨ Result: Improved reliability and coverage for Firebase utility functions.
PR created automatically by Jules for task 4205446835027658903 started by @DaTiC0
Summary by Sourcery
Add comprehensive tests for Firebase utility reference behavior and mock data handling.
Tests: