🧪 [testing improvement] Add missing error test for load_settings in core.py#93
🧪 [testing improvement] Add missing error test for load_settings in core.py#93skurtyyskirts wants to merge 1 commit into
Conversation
…ore.py Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adds a regression test to ensure core.RemixConnectorPlugin.load_settings() handles failures reading settings.json by logging an error and continuing with fallback settings.
Changes:
- Added
tests/test_core.pywith a unit test exercising theload_settings()exception path via mocking. - Introduced import-time mocks for Qt-related dependencies so
core.pycan be imported in a standard CI environment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return m | ||
| elif name == 'settings_schema': | ||
| m = MagicMock() | ||
| m.sanitize_settings = lambda x, y: x |
| # Clean up added modules from sys.modules to prevent pollution | ||
| if 'core' in sys.modules: | ||
| del sys.modules['core'] | ||
|
|
||
| if hasattr(cls, 'original_modules'): | ||
| keys_to_remove = [k for k in sys.modules if k not in cls.original_modules] | ||
| for k in keys_to_remove: | ||
| del sys.modules[k] | ||
| sys.modules.update(cls.original_modules) | ||
|
|
| self.assertTrue( | ||
| "Failed to load settings (using defaults)" in plugin.log_error.call_args[0][0] | ||
| ) |
| @classmethod | ||
| def _cleanup(cls): | ||
| builtins.__import__ = getattr(cls, 'original_import', builtins.__import__) | ||
|
|
||
| # Clean up added modules from sys.modules to prevent pollution | ||
| if 'core' in sys.modules: | ||
| del sys.modules['core'] | ||
|
|
||
| if hasattr(cls, 'original_modules'): | ||
| keys_to_remove = [k for k in sys.modules if k not in cls.original_modules] | ||
| for k in keys_to_remove: | ||
| del sys.modules[k] | ||
| sys.modules.update(cls.original_modules) | ||
|
|
| # Replace the logging method to observe if it logs the error correctly | ||
| plugin.log_error = MagicMock() | ||
|
|
||
| # Force os.path.exists to raise an Exception inside the try block of load_settings | ||
| with patch('os.path.exists', side_effect=Exception("Simulated error reading settings")): | ||
| plugin.load_settings() | ||
|
|
||
| # Verify log_error was called with the correct message indicating fallback | ||
| plugin.log_error.assert_called_once() | ||
| self.assertTrue( | ||
| "Failed to load settings (using defaults)" in plugin.log_error.call_args[0][0] | ||
| ) | ||
| # Verify the settings are successfully sanitized into an empty dict because of the mock lambda x,y:x | ||
| # and the fallback raw={} in core.py | ||
| self.assertEqual(plugin.settings, {}) |
🎯 What: Added missing error test for core.py's load_settings method to ensure it catches exceptions and falls back to default settings.
📊 Coverage: The error scenario when reading settings.json fails is now tested via mocking.
✨ Result: Improved test coverage and reliability of application fallback behavior when the config file is unreadable.
PR created automatically by Jules for task 9883747851714888057 started by @skurtyyskirts