Conversation
- Mock sys.modules['bids'] before importing src.run to prevent ModuleNotFoundError in CI - Remove problematic patch context manager that tried to import non-existent bids module - Tests now work in environments without pybids installed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Handle ModuleNotFoundError when pkg_resources is not available (Python 3.12+) - Create mock pkg_resources when import fails - Add pkg_resources to sys.modules for proper test patching - Tests now work across Python 3.9-3.12+ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @yibeichan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses continuous integration failures by enhancing the test setup in "tests/test_run.py" to improve compatibility with Python 3.12+ and strengthen the mocking of external dependencies. The changes specifically handle the deprecation of "pkg_resources" and ensure that the "bids.BIDSLayout" class is reliably mocked, leading to more robust and future-proof tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the CI failures on Python 3.12+ by handling the deprecation of pkg_resources and improving the mocking strategy for external dependencies in the tests. The changes are well-focused on the test setup and appear correct.
My review includes a couple of suggestions for improving maintainability. One comment points out some redundancy created by the new mocking strategy that could be cleaned up. Another suggests a more readable way to create a mock exception class.
As a future improvement, you might consider migrating src/run.py to use importlib.metadata (available since Python 3.8) instead of pkg_resources. This would remove the dependency on the deprecated library from the application code itself, providing a more permanent solution.
| except ModuleNotFoundError: | ||
| # Create a mock pkg_resources for testing purposes | ||
| pkg_resources = MagicMock() | ||
| pkg_resources.DistributionNotFound = type('DistributionNotFound', (Exception,), {}) |
There was a problem hiding this comment.
Using type() to dynamically create an exception class is a bit unconventional and can harm readability. For better clarity and maintainability, it's preferable to use a standard class definition. This makes the intent clearer to developers who might work on this code in the future.
For example:
class DistributionNotFound(Exception):
pass
pkg_resources.DistributionNotFound = DistributionNotFound| # Mock bids module | ||
| mock_bids = MagicMock() | ||
| mock_bids.BIDSLayout = MagicMock(return_value=mock_layout) | ||
|
|
||
| # Configure the mocks | ||
| sys.modules['ants'] = mock_ants | ||
| sys.modules['numpy'] = np | ||
| sys.modules['nibabel'] = MagicMock() | ||
| sys.modules['bids'] = mock_bids | ||
| sys.modules['pkg_resources'] = pkg_resources |
There was a problem hiding this comment.
This change introduces a more robust way of mocking bids and pkg_resources by manipulating sys.modules, which is a good improvement. However, this new global mock for bids.BIDSLayout makes the local patch('bids.BIDSLayout') calls within test_process_participant (line 278) and test_process_session (line 390) redundant. To improve code clarity and reduce complexity, you should remove those now-unnecessary patches.
1 participant
This pull request updates the test setup in
tests/test_run.pyto improve compatibility with Python 3.12+ and strengthen the mocking of external dependencies. The main focus is handling the deprecation ofpkg_resourcesand ensuring thatbids.BIDSLayoutis properly mocked for test reliability.Python compatibility and dependency mocking:
pkg_resources, providing a mock implementation when the module is not available, which is necessary for Python 3.12+ compatibility.bidsmodule and itsBIDSLayoutclass, and registered bothbidsandpkg_resourcesmocks insys.modulesto ensure that downstream imports use the mocked versions.