refactor: Make warnings error in tests #1192
Conversation
Reviewer's Guide by SourceryThis pull request focuses on changing warnings into errors in tests and fixing code that emits warnings. The changes primarily involve updating the Sentry SDK usage, modifying pytest configurations, and addressing deprecated functions in various parts of the codebase. File-Level Changes
Tips
|
WalkthroughThe pull request introduces enhancements to type annotations and color handling in the Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Czaki - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR title and description don't fully capture the scope of changes. Consider providing more details about the updates to sentry-sdk usage, numpy/pandas changes, and any configuration changes for turning warnings into errors.
- The change from
np.fromstring()tonp.frombuffer().copy()is a good update. Consider mentioning this explicitly in the PR description as it addresses a deprecation warning.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| array = array.view(dtype) | ||
| else: | ||
| array = np.fromstring(imagecodecs.zstd_decode(data[header.header_size :]), dtype) | ||
| array = np.frombuffer(imagecodecs.zstd_decode(data[header.header_size :]), dtype).copy() |
There was a problem hiding this comment.
suggestion (performance): Profile the performance impact of added .copy() operations
While changing from fromstring to frombuffer is good, the added .copy() might impact performance. Consider profiling this change and evaluate if a copy-on-write approach could be more efficient in some scenarios.
| array = np.frombuffer(imagecodecs.zstd_decode(data[header.header_size :]), dtype).copy() | |
| array = np.frombuffer(imagecodecs.zstd_decode(data[header.header_size :]), dtype) | |
| if array.flags['WRITEABLE']: | |
| return array | |
| return array.copy() |
There was a problem hiding this comment.
I know that this buffer is not writable
| assert len(clipped["exception"]["values"][0]["stacktrace"]["frames"][0]["vars"]) == len(vars_dict) | ||
| assert len(clipped["exception"]["values"][0]["stacktrace"]["frames"][0]["vars"]) > 10 | ||
| client = Client("https://aaa@test.pl/77") | ||
| Hub.current.bind_client(client) |
There was a problem hiding this comment.
suggestion (testing): Update test to use new_scope() instead of push_scope()
The test has been updated to use sentry_sdk.new_scope() instead of sentry_sdk.push_scope(). This change reflects updates in the Sentry SDK. Ensure that all other tests using Sentry scopes are updated similarly for consistency.
| assert "body=This" in mock_web.call_args.args[0] | ||
|
|
||
| @patch("requests.post") | ||
| @patch("sentry_sdk.push_scope") |
There was a problem hiding this comment.
suggestion (testing): Update mock to use new_scope instead of push_scope
The test has been updated to mock sentry_sdk.new_scope() instead of sentry_sdk.push_scope(). This change aligns with the updates in the Sentry SDK usage. Ensure all other tests mocking Sentry scopes are updated similarly for consistency.
|
|
||
| def test_project_to_layers_roi(): | ||
| data = np.zeros((1, 1, 10, 10, 10), dtype=np.uint8) | ||
| img = PImage(data, image_spacing=(1, 1, 1), name="ROI") |
There was a problem hiding this comment.
suggestion (testing): Add axes_order parameter to PImage in test_project_to_layers_roi
The test has been updated to include the axes_order parameter when creating a PImage object. This change likely reflects an update in the PImage class or a more explicit test setup. Consider adding assertions to verify that the axes order is correctly handled in the resulting layers.
img = PImage(data, image_spacing=(1, 1, 1), name="ROI", axes_order="CTZYX")
assert img.axes_order == "CTZYX"
assert res.image.axes_order == "CTZYX"
There was a problem hiding this comment.
axes_order argument is to told of axes order of data array.
There was a problem hiding this comment.
Thank you for your feedback, we will generate more comments like this in the future according to the following instructions:
- Ensure the comment suggests a meaningful improvement or addition to the test, such as adding parameters or assertions.
- Verify that the comment provides a clear rationale for the suggested change, explaining its relevance or necessity.
- Check that the comment includes specific code examples or snippets to illustrate the suggestion.
- Confirm that the comment is related to the functionality or correctness of the code being tested.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Czaki
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
pyproject.toml (1)
209-223: Suppressingpydanticdeprecation warnings is fine for now, but create GitHub issues to track and address them.The added filters suppress various deprecation warnings related to the
pydanticlibrary, allowing the tests to pass without being affected by the deprecated features or syntax.However, suppressing these warnings may mask potential issues related to the use of deprecated features or syntax in the codebase. It is important to gradually address these deprecations and update the codebase to use the recommended alternatives to ensure future compatibility and maintainability.
Do you want me to open GitHub issues to track the suppressed deprecations and plan their resolution?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pyproject.toml (3 hunks)
Additional comments not posted (4)
pyproject.toml (4)
64-64: LGTM!Constraining the
pydanticdependency to versions less than3ensures compatibility with existing code while allowing for updates within the major version.
67-67: LGTM!Upgrading the
requestsdependency to version2.25.0allows for minor and patch version updates while staying within the same major version, which is unlikely to introduce any breaking changes.
69-69: Verify the impact of the major version upgrade on error tracking and reporting.The
sentry-sdkdependency has been significantly upgraded from version0.14.3to2.4.0. This major version bump may introduce new features, changes in behavior, or breaking changes that could impact the application's error tracking and reporting functionality.Please ensure that:
- The application's error tracking and reporting continue to function as expected after the upgrade.
- Any necessary changes in the codebase have been made to adapt to breaking changes or to utilize new features introduced in the newer versions of
sentry-sdk.
194-195: LGTM! But be prepared to address warnings in existing tests.Escalating warnings to errors in the
PartSegmodule and tests indicates a more stringent approach to handling warnings during testing. This change ensures that potential issues are addressed more rigorously and are not overlooked as mere warnings.However, please note that this change may cause existing tests to fail if they currently generate warnings. Be prepared to address these warnings by making necessary code changes in the
PartSegmodule and tests to eliminate the warning-generating code or to explicitly handle the warnings.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1192 +/- ##
===========================================
+ Coverage 93.05% 93.07% +0.01%
===========================================
Files 209 209
Lines 32909 32927 +18
===========================================
+ Hits 30625 30646 +21
+ Misses 2284 2281 -3 ☔ View full report in Codecov by Sentry. |
|
1 participant
Change
pytestconfiguration to change warnings into errors in the tests.Fix code that emites warning.
Summary by Sourcery
Convert warnings to errors in tests by updating the
pytestconfiguration and refactor code to handle warnings appropriately. Enhance thesentry_sdkusage by replacingpush_scopewithnew_scope. Improve data handling inbatch_backend.pyand update tests to align with these changes.Bug Fixes:
test_no_updatetest to correctly handle URL errors by usingurllib.error.URLError.test_ignore_file_exists_old_datetest to assert the correct release version and URL.Enhancements:
sentry_sdkusage to replacepush_scopewithnew_scopefor better scope management.CheckVersionThreadinitialization by updating the default URL to a more relevant one.get_data_to_writemethod inbatch_backend.pyto handle empty data frames more efficiently.decode_zstd1anddecode_zstd0functions to usenp.frombufferinstead ofnp.fromstringfor better performance and compatibility.Tests:
pytestconfiguration.sentry_sdkscope management.Summary by CodeRabbit
Release Notes
New Features
napariby adjusting colormap settings.Bug Fixes
Documentation
Tests
naparito ensure correct behavior based on library version.