-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Improve test suite for PartSegCore
#1077
Conversation
PartSegCore
PartSegCore
WalkthroughThe recent updates across the PartSegCore package enhance its functionality and maintainability. Key improvements include the extension of base classes for better I/O handling, the introduction of deprecation warnings for outdated functions, and the refinement of exception handling in plugins. Additionally, the test suite has seen significant expansion, ensuring robustness through new tests for algorithms, I/O operations, and plugin integrations. These changes collectively aim to streamline workflows, encourage best practices, and prepare the codebase for future developments. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Refactoring
PR Summary: This pull request introduces several refactoring changes to the PartSegCore
package, focusing on improving code maintainability and readability. Key changes include the introduction of an _IOBase
class to reduce duplication in SaveBase
and LoadBase
classes, enhancements in error handling and extension parsing in io_utils.py
, and the removal of unused code in algorithm_describe_base.py
. Additionally, the PR extends the type support in the load_metadata_base
function and adds comprehensive tests for new and existing functionalities.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that all new error messages and exceptions provide enough context for debugging and are consistent across the codebase.
- Verify the robustness of regex patterns used for parsing to handle all expected input formats effectively.
- Consider adding more detailed docstrings to newly introduced methods and classes to improve code documentation and maintainability.
- Review the removal of code segments to ensure that no functionality is inadvertently affected, especially in areas where deprecated or unused code is removed.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
@@ -42,7 +43,7 @@ def check_segmentation_type(tar_file: TarFile) -> SegmentationType: | |||
return SegmentationType.analysis | |||
if "metadata.json" in names: | |||
return SegmentationType.mask | |||
raise WrongFileTypeException | |||
raise WrongFileTypeException # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): Consider providing a specific error message to WrongFileTypeException
to enhance error traceability and debugging experience.
raise ValueError(f"No extensions found in {cls.get_name()}") | ||
extensions = match[1].split(" ") | ||
if not all(x.startswith("*.") for x in extensions): | ||
raise ValueError(f"Error with parsing extensions in {cls.get_name()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): The error message for parsing extensions is clear, but consider including the incorrect extension format in the message to aid in debugging.
return [x[1:] for x in extensions] | ||
def get_default_extension(cls): | ||
match = re.search(r"\(\*(\.\w+)", cls.get_name_with_suffix()) | ||
return match[1] if match else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (llm): The use of regex for extracting the default extension is efficient. However, ensure that the regex pattern is robust enough to handle all expected formats of get_name_with_suffix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (4)
- package/PartSegCore/algorithm_describe_base.py (2 hunks)
- package/PartSegCore/io_utils.py (7 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (3 hunks)
- package/tests/test_PartSegCore/test_io.py (7 hunks)
Additional comments: 17
package/tests/test_PartSegCore/test_algorithm_describe_base.py (2)
- 36-38: The test
test_algorithm_property_no_kwargs
effectively validates the handling of unexpected keyword arguments. This is a good practice for ensuring robustness.- 102-106: The assertion
assert TestSelection.__register__ != TestSelection2.__register__
is crucial for ensuring that registrations are isolated between subclasses. This is a good practice for maintainability and avoiding side effects.package/PartSegCore/io_utils.py (2)
- 1-1: The addition of the
io
import is appropriate for the subsequent use ofio.TextIOBase
in type checks.- 184-186: The modification to
load_metadata_base
to acceptio.TextIOBase
as an input type is a good enhancement for flexibility in data sources. Ensure that all callers of this function are updated to handle the broader range of input types.package/PartSegCore/algorithm_describe_base.py (7)
- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
Enum
import is not directly visible in the provided code. Ensure that no remaining code referencesEnum
to avoid breaking changes.Verification successful
The script execution did not find any references to
Enum
in thealgorithm_describe_base.py
file, indicating that the removal of theEnum
import has been handled correctly, with no remaining references that could lead to breaking changes. This aligns with the review comment's concern about ensuring no code is broken due to the removal.* 3-8: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to Enum in the algorithm_describe_base.py file. rg "Enum" package/PartSegCore/algorithm_describe_base.pyLength of output: 56
The
print_dict
method removal from theAlgorithmDescribeBase
class is mentioned but not visible in the provided code. Confirm that its removal does not impact any functionality where it might have been used.Verification successful
The search for references to the
print_dict
method within the PartSegCore project did not yield any results, indicating that the method is not directly referenced in the codebase. This suggests that its removal from theAlgorithmDescribeBase
class likely does not impact any functionality within the PartSegCore package. However, it's important to note that this conclusion is based on the absence of direct references found through text search, and does not rule out the possibility of dynamic invocation or other indirect references.* 3-8: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to the print_dict method within the PartSegCore project. rg "print_dict" package/PartSegCore/Length of output: 36
The file imports several modules and classes. Verify that all imported entities are used within the file to maintain cleanliness and avoid unnecessary imports.
Verification successful
This script iterates over a curated list of unique imports, checking for their usage within
algorithm_describe_base.py
, excluding the import statements themselves.
This corrected script ensures that the verification process checks the usage of each imported entity within the correct file,
algorithm_describe_base.py
, by specifying the exact file path.
The verification process has revealed that all imported entities in
algorithm_describe_base.py
are indeed utilized within the file. Each import listed from the script's output has shown usage outside of its import statement, indicating that the original concern about maintaining cleanliness and avoiding unnecessary imports has been addressed. Therefore, the imports inalgorithm_describe_base.py
are justified and serve a purpose within the file.* 3-8: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual verification needed to ensure all imports are utilized within algorithm_describe_base.py. echo "Manually verify that all imports are used within the file."Length of output: 126
Script:
#!/bin/bash # List all imports from algorithm_describe_base.py fd algorithm_describe_base.py --exec rg '^import|^from' {}Length of output: 1033
Script:
#!/bin/bash # Verify the usage of each imported entity within algorithm_describe_base.py # List of unique imports to check for usage, considering potential duplicates and aliases imports=( "typing" "Enum" "pytest" "class_to_str" "register_class" "PydanticBaseModel" "Field" "ValidationError" "AnalysisAlgorithmSelection" "LowerThresholdAlgorithm" "BaseModel" "Channel" "inspect" "textwrap" "warnings" "ABC" "ABCMeta" "abstractmethod" "wraps" "REGISTER" "create_model" "validator" "ModelField" "UndefinedType" "ModelMetaclass" "Annotated" ) file_path=$(fd algorithm_describe_base.py) for import in "${imports[@]}"; do echo "Checking usage of: $import" rg --vimgrep --count "$import" "$file_path" | rg -v '^import |^from' doneLength of output: 11522
Script:
#!/bin/bash # Corrected script to verify the usage of each imported entity within algorithm_describe_base.py, focusing on the correct file. # List of unique imports to check for usage, considering potential duplicates and aliases imports=( "typing" "Enum" "pytest" "class_to_str" "register_class" "PydanticBaseModel" "Field" "ValidationError" "AnalysisAlgorithmSelection" "LowerThresholdAlgorithm" "BaseModel" "Channel" "inspect" "textwrap" "warnings" "ABC" "ABCMeta" "abstractmethod" "wraps" "REGISTER" "create_model" "validator" "ModelField" "UndefinedType" "ModelMetaclass" "Annotated" ) # Correctly identifying the file path for algorithm_describe_base.py file_path="package/PartSegCore/algorithm_describe_base.py" for import in "${imports[@]}"; do echo "Checking usage of: $import" rg --vimgrep --count "$import" "$file_path" | rg -v '^import |^from' doneLength of output: 6864
The file defines multiple classes and functions. Ensure that each class and function adheres to the Single Responsibility Principle for maintainability and modularity.
- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The use of
warnings.warn
for deprecation and future changes is good practice. Ensure that all deprecated features have a clear migration path or alternative for users.Verification successful
The examination of the codebase reveals numerous instances where
warnings.warn
is used to indicate deprecated features or practices. Many of these instances include messages that either directly suggest alternatives or indicate a change in usage, which aligns with the best practice of providing clear migration paths or alternatives for deprecated features. While not every deprecation warning may detail a migration path explicitly, the presence of these warnings and the guidance provided in many of them suggest a general adherence to informing users about deprecations and guiding them towards newer practices. This approach appears to align with the intent of ensuring that all deprecated features have clear migration paths or alternatives for users.* 3-8: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual verification needed to ensure clear migration paths for deprecated features. echo "Manually verify clear migration paths for deprecated features."Length of output: 134
Script:
#!/bin/bash # Search for usage of warnings.warn in the codebase to identify deprecation warnings. rg "warnings\.warn" --context 5Length of output: 35918
The use of
typing.cast
for type casting is appropriate. Ensure that all type casts are necessary and correct to prevent runtime type errors.
- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file makes extensive use of custom exceptions, such as
AlgorithmDescribeNotFound
. Ensure that these exceptions are caught and handled appropriately wherever they might be raised.package/tests/test_PartSegCore/test_io.py (6)
- 32-32: The
LoadPoints
entity has been added. Ensure it integrates well with existing I/O functionalities and follows the project's coding standards.- 307-307: The
UPDATE_NAME_JSON
constant is used withintest_update_name
. Verify that the JSON structure aligns with expected measurement profiles and that the test accurately reflects changes in measurement handling.- 486-511: The
test_mask_project_tuple
method correctly tests the functionality ofMaskProjectTuple
. Ensure that the test covers all relevant aspects of masking and project tuple handling, including edge cases and error handling.- 793-800: The test
test_save_base_extension_parse_no_ext
checks for an error when no extensions are provided. This is a good practice to ensure robustness in handling file extensions.- 803-810: The test
test_save_base_extension_parse_malformated_ext
checks for parsing errors in file extensions. This is important for maintaining the integrity of file handling operations.- 813-823: The
LoadPoints
test case ensures that the loading functionality for point data from CSV files works as expected. Verify that the test covers all scenarios, including malformed data and edge cases.
def test_algorithm_property(): | ||
ap = AlgorithmProperty("test", "Test", 1) | ||
assert ap.name == "test" | ||
assert "user_name='Test'" in repr(ap) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the test test_algorithm_property
includes assertions for all relevant properties of AlgorithmProperty
to fully validate its behavior.
def test_algorithm_property_warn(): | ||
with pytest.warns(DeprecationWarning, match="use value_type instead"): | ||
ap = AlgorithmProperty("test", "Test", 1, property_type=int) | ||
assert ap.value_type == int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_algorithm_property_warn
correctly checks for a deprecation warning. Consider adding a comment explaining the context of deprecation for clarity.
def test_algorithm_property_list_exc(): | ||
with pytest.raises(ValueError, match="should be one of possible values"): | ||
AlgorithmProperty("test", "Test", 1, possible_values=[2, 3], value_type=list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test_algorithm_property_list_exc
, validate the exception message more precisely to ensure it matches the expected error scenario closely.
prof1 = ROIExtractionProfile(name="", algorithm="aaa", values={}) | ||
assert f"{prof1}\n " == prof1.pretty_print(AnalysisAlgorithmSelection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_pretty_print
with an empty name
field in ROIExtractionProfile
seems redundant as it does not assert any specific behavior related to the empty name
. Consider either removing it or clarifying its purpose.
class _IOBase(AlgorithmDescribeBase, ABC): | ||
@classmethod | ||
def get_name_with_suffix(cls): | ||
return cls.get_name() | ||
|
||
@classmethod | ||
def get_extensions(cls) -> typing.List[str]: | ||
match = re.match(r".*\((.*)\)", cls.get_name()) | ||
if match is None: | ||
raise ValueError(f"No extensions found in {cls.get_name()}") | ||
extensions = match[1].split(" ") | ||
if not all(x.startswith("*.") for x in extensions): | ||
raise ValueError(f"Error with parsing extensions in {cls.get_name()}") | ||
return [x[1:] for x in extensions] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods get_name_with_suffix
and get_extensions
in _IOBase
introduce new functionality for handling file extensions. Ensure that the regular expression used in get_extensions
is robust and correctly handles all expected formats.
def get_default_extension(cls): | ||
match = re.search(r"\(\*(\.\w+)", cls.get_name_with_suffix()) | ||
return match[1] if match else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method get_default_extension
in SaveBase
uses a regular expression to extract the default file extension. Verify that this method correctly handles edge cases and document any assumptions or limitations.
def test_load_workflow_from_text(self, bundle_test_dir): | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
data_text = ff.read() | ||
assert isinstance(load_metadata_base(data_text)["workflow"], CalculationPlan) | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
isinstance(load_metadata_base(ff)["workflow"], CalculationPlan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is loading same file twice, but in a different way. First by passing file content, second by passing TextIOWrapper. It validates if a function handles all input types properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Czaki: Thank you for the clarification. I understand now that the method is intentionally designed to test the load_metadata_base
function with different types of inputs, ensuring it handles both file content and TextIOWrapper correctly. This is a thoughtful approach to testing the function's flexibility.
import typing | ||
import warnings | ||
from abc import ABC, ABCMeta, abstractmethod | ||
from enum import Enum | ||
from functools import wraps | ||
|
||
from local_migrator import REGISTER, class_to_str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file contains complex classes with multiple methods and properties. Consider adding comprehensive docstrings to public methods and classes to improve maintainability and understandability.
+ Add comprehensive docstrings to public methods and classes.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The dynamic creation of models using create_model
in _GetDescriptionClass
is an advanced feature. Ensure that this dynamic behavior is well-documented and tested to avoid confusion and potential bugs.
+ Improve documentation and testing for dynamic model creation in _GetDescriptionClass.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The implementation of custom metaclasses, such as AlgorithmDescribeBaseMeta
and AddRegisterMeta
, introduces complexity. Verify that their use is justified and consider simplifying the design if possible.
+ Evaluate the necessity of custom metaclasses and consider simplifying the design.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1077 +/- ##
===========================================
+ Coverage 92.31% 92.78% +0.46%
===========================================
Files 205 204 -1
Lines 32351 32315 -36
===========================================
+ Hits 29864 29982 +118
+ Misses 2487 2333 -154 ☔ View full report in Codecov by Sentry. |
def test_load_workflow_from_text(self, bundle_test_dir): | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
data_text = ff.read() | ||
assert isinstance(load_metadata_base(data_text)["workflow"], CalculationPlan) | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
isinstance(load_metadata_base(ff)["workflow"], CalculationPlan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is loading same file twice, but in a different way. First by passing file content, second by passing TextIOWrapper. It validates if a function handles all input types properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- package/PartSegCore/algorithm_describe_base.py (2 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- package/PartSegCore/algorithm_describe_base.py
- package/tests/test_PartSegCore/test_algorithm_describe_base.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (2)
- package/PartSegCore/algorithm_describe_base.py (8 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/PartSegCore/algorithm_describe_base.py
Additional comments: 3
package/tests/test_PartSegCore/test_algorithm_describe_base.py (3)
- 24-28: Ensure the test
test_algorithm_property
includes assertions for all relevant properties ofAlgorithmProperty
to fully validate its behavior.- 30-33: The test
test_algorithm_property_warn
correctly checks for a deprecation warning. Consider adding a comment explaining the context of deprecation for clarity.- 41-43: In
test_algorithm_property_list_exc
, validate the exception message more precisely to ensure it matches the expected error scenario closely.
assert TestSelection.__register__ != TestSelection2.__register__ | ||
|
||
ts = TestSelection(name="test1", values={}) | ||
assert ts.algorithm() == Class1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion in lines 102 and 105 seems redundant since the uniqueness of __register__
between TestSelection
and TestSelection2
is implicitly tested by the assertion on line 102. Consider removing line 105 to streamline the test.
- ts = TestSelection(name="test1", values={})
- assert ts.algorithm() == Class1
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
assert TestSelection.__register__ != TestSelection2.__register__ | |
ts = TestSelection(name="test1", values={}) | |
assert ts.algorithm() == Class1 | |
assert TestSelection.__register__ != TestSelection2.__register__ |
with pytest.raises(ValueError, match="Class .* need to define classmethod 'get_name'"): | ||
TestSelection.register(Alg1) | ||
|
||
with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"): | ||
TestSelection.register(Alg2) | ||
|
||
with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"): | ||
TestSelection.__register__["test1"] = Alg2 | ||
|
||
with pytest.raises(ValueError, match="Function get_name of class .* need return string not .*int"): | ||
TestSelection.register(Alg3) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message validation in test_register_errors
should include specific class names or error details to ensure the test is accurately capturing the intended failure scenarios.
- with pytest.raises(ValueError, match="Class .* need to define classmethod 'get_name'"):
+ with pytest.raises(ValueError, match="Class Alg1 need to define classmethod 'get_name'"):
- with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"):
+ with pytest.raises(ValueError, match="Class Alg2 need to implement classmethod 'get_name'"):
- with pytest.raises(ValueError, match="Function get_name of class .* need return string not .*int"):
+ with pytest.raises(ValueError, match="Function get_name of class Alg3 need return string not int"):
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
with pytest.raises(ValueError, match="Class .* need to define classmethod 'get_name'"): | |
TestSelection.register(Alg1) | |
with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"): | |
TestSelection.register(Alg2) | |
with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"): | |
TestSelection.__register__["test1"] = Alg2 | |
with pytest.raises(ValueError, match="Function get_name of class .* need return string not .*int"): | |
TestSelection.register(Alg3) | |
with pytest.raises(ValueError, match="Class Alg1 need to define classmethod 'get_name'"): | |
TestSelection.register(Alg1) | |
with pytest.raises(ValueError, match="Class Alg2 need to implement classmethod 'get_name'"): | |
TestSelection.register(Alg2) | |
with pytest.raises(ValueError, match="Class .* need to implement classmethod 'get_name'"): | |
TestSelection.__register__["test1"] = Alg2 | |
with pytest.raises(ValueError, match="Function get_name of class Alg3 need return string not int"): | |
TestSelection.register(Alg3) | |
TestSelection.register(Alg1, old_names=["0"]) | ||
with pytest.raises( | ||
ValueError, match="Object .* with this name: '1' already exist and register is not in replace mode" | ||
): | ||
TestSelection.register(Alg2) | ||
|
||
assert len(TestSelection.__register__) == 1 | ||
|
||
with pytest.raises(ValueError, match="Old value mapping for name '0' already registered"): | ||
TestSelection.register(Alg3, old_names=["0"]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_register_name_collision
correctly checks for name collisions but could be improved by adding a successful registration assertion after the failed ones to demonstrate that the register works correctly when not in collision.
+ # Successful registration after handling collisions
+ TestSelection.register(Alg3)
+ assert "2" in TestSelection.__register__
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
TestSelection.register(Alg1, old_names=["0"]) | |
with pytest.raises( | |
ValueError, match="Object .* with this name: '1' already exist and register is not in replace mode" | |
): | |
TestSelection.register(Alg2) | |
assert len(TestSelection.__register__) == 1 | |
with pytest.raises(ValueError, match="Old value mapping for name '0' already registered"): | |
TestSelection.register(Alg3, old_names=["0"]) | |
TestSelection.register(Alg1, old_names=["0"]) | |
with pytest.raises( | |
ValueError, match="Object .* with this name: '1' already exist and register is not in replace mode" | |
): | |
TestSelection.register(Alg2) | |
assert len(TestSelection.__register__) == 1 | |
with pytest.raises(ValueError, match="Old value mapping for name '0' already registered"): | |
TestSelection.register(Alg3, old_names=["0"]) | |
# Successful registration after handling collisions | |
TestSelection.register(Alg3) | |
assert "2" in TestSelection.__register__ |
with pytest.raises(ValueError, match="Class .* need to be subclass of .*AlgorithmDescribeBase"): | ||
TestSelection.register(Alg1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message in test_register_not_subclass
should specify that Alg1
is not a subclass of AlgorithmDescribeBase
for clearer test failure messages.
- with pytest.raises(ValueError, match="Class .* need to be subclass of .*AlgorithmDescribeBase"):
+ with pytest.raises(ValueError, match="Class Alg1 need to be subclass of AlgorithmDescribeBase"):
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
with pytest.raises(ValueError, match="Class .* need to be subclass of .*AlgorithmDescribeBase"): | |
TestSelection.register(Alg1) | |
with pytest.raises(ValueError, match="Class Alg1 need to be subclass of AlgorithmDescribeBase"): | |
TestSelection.register(Alg1) |
with pytest.raises(ValueError, match="need return string"): | ||
TestSelection.__register__["1"] = Alg2 | ||
|
||
with pytest.raises(ValueError, match="under name returned by get_name function"): | ||
TestSelection.__register__["2"] = Alg1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message checks in test_register_validate_name_assignment
could be more specific about the expected string format and the reason for the failure.
- with pytest.raises(ValueError, match="need return string"):
+ with pytest.raises(ValueError, match="Class Alg2's get_name method must return a string, got int instead"):
- with pytest.raises(ValueError, match="under name returned by get_name function"):
+ with pytest.raises(ValueError, match="Class Alg1 cannot be registered under '2', as its get_name method returns '1'"):
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
with pytest.raises(ValueError, match="need return string"): | |
TestSelection.__register__["1"] = Alg2 | |
with pytest.raises(ValueError, match="under name returned by get_name function"): | |
TestSelection.__register__["2"] = Alg1 | |
with pytest.raises(ValueError, match="Class Alg2's get_name method must return a string, got int instead"): | |
TestSelection.__register__["1"] = Alg2 | |
with pytest.raises(ValueError, match="Class Alg1 cannot be registered under '2', as its get_name method returns '1'"): | |
TestSelection.__register__["2"] = Alg1 |
with pytest.raises(ValueError, match="need to be implemented"): | ||
TestSelection.register(Alg1) | ||
with pytest.raises(ValueError, match="need return list not"): | ||
TestSelection.register(Alg2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test_register_get_fields_validity
, the error message for Alg1
should explicitly state that get_fields
must not raise NotImplementedError
.
- with pytest.raises(ValueError, match="need to be implemented"):
+ with pytest.raises(ValueError, match="Alg1's get_fields method must be implemented and not raise NotImplementedError"):
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
with pytest.raises(ValueError, match="need to be implemented"): | |
TestSelection.register(Alg1) | |
with pytest.raises(ValueError, match="need return list not"): | |
TestSelection.register(Alg2) | |
with pytest.raises(ValueError, match="Alg1's get_fields method must be implemented and not raise NotImplementedError"): | |
TestSelection.register(Alg1) | |
with pytest.raises(ValueError, match="need return list not"): | |
TestSelection.register(Alg2) |
|
||
prof1 = ROIExtractionProfile(name="aaa", algorithm="Lower threshold", values={}) | ||
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile name:") | ||
prof1 = ROIExtractionProfile(name="", algorithm="Lower threshold", values={}) | ||
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_pretty_print
effectively checks the output format of pretty_print
but consider adding more detailed assertions to verify the content of the output, especially for prof2
where multiple lines are expected.
+ assert "Lower threshold" in prof2.pretty_print(AnalysisAlgorithmSelection)
+ assert "default values" in prof2.pretty_print(AnalysisAlgorithmSelection)
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
prof1 = ROIExtractionProfile(name="aaa", algorithm="Lower threshold", values={}) | |
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile name:") | |
prof1 = ROIExtractionProfile(name="", algorithm="Lower threshold", values={}) | |
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile\n") | |
prof1 = ROIExtractionProfile(name="aaa", algorithm="Lower threshold", values={}) | |
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile name:") | |
prof1 = ROIExtractionProfile(name="", algorithm="Lower threshold", values={}) | |
assert prof1.pretty_print(AnalysisAlgorithmSelection).startswith("ROI extraction profile\n") | |
assert "Lower threshold" in prof2.pretty_print(AnalysisAlgorithmSelection) | |
assert "default values" in prof2.pretty_print(AnalysisAlgorithmSelection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (4)
- package/PartSegCore/io_utils.py (10 hunks)
- package/PartSegCore/mask/io_functions.py (1 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (3 hunks)
- package/tests/test_PartSegCore/test_io.py (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/PartSegCore/io_utils.py
Additional comments: 24
package/tests/test_PartSegCore/test_algorithm_describe_base.py (10)
- 24-28: Ensure to include assertions for all relevant properties of
AlgorithmProperty
to fully validate its behavior.- 30-33: Consider adding a comment explaining the context of deprecation for clarity.
- 41-43: Validate the exception message more precisely to ensure it matches the expected error scenario closely.
- 102-106: The assertion in lines 102 and 105 seems redundant since the uniqueness of
__register__
betweenTestSelection
andTestSelection2
is implicitly tested by the assertion on line 102. Consider removing line 105 to streamline the test.- 127-138: The error message validation in
test_register_errors
should include specific class names or error details to ensure the test is accurately capturing the intended failure scenarios.- 171-181: The test
test_register_name_collision
correctly checks for name collisions but could be improved by adding a successful registration assertion after the failed ones to demonstrate that the register works correctly when not in collision.- 196-197: The error message in
test_register_not_subclass
should specify thatAlg1
is not a subclass ofAlgorithmDescribeBase
for clearer test failure messages.- 218-222: The error message checks in
test_register_validate_name_assignment
could be more specific about the expected string format and the reason for the failure.- 243-246: In
test_register_get_fields_validity
, the error message forAlg1
should explicitly state thatget_fields
must not raiseNotImplementedError
.- 541-545: The test
test_pretty_print
effectively checks the output format ofpretty_print
but consider adding more detailed assertions to verify the content of the output, especially forprof2
where multiple lines are expected.package/PartSegCore/mask/io_functions.py (1)
- 109-109: The use of
# pragma: no cover
correctly excludes the deprecatedroi
method from test coverage. However, ensure that the deprecation plan includes migrating existing usages to the new approach and eventually removing this method.package/tests/test_PartSegCore/test_io.py (13)
- 11-11: BytesIO import added without direct usage in the provided code changes.
- 33-33: New entity
LoadPoints
added. Ensure it's correctly implemented and used where necessary.- 35-35: New entity
SaveMaskAsTiff
added. Verify its implementation and usage.- 37-37: New entity
SaveScreenshot
added. Confirm its implementation and usage.- 42-42: New entity
open_tar_file
added. Check its implementation and usage.- 48-48: New entity
LoadROIFromTIFF
added. Ensure its correct implementation and usage.- 340-345: The method
test_load_workflow_from_text
reads a workflow JSON file twice but only asserts the type of loaded data in one instance. Ensure consistency in testing and consider removing redundant file reading or adding relevant assertions.- 495-520: The
test_mask_project_tuple
method correctly tests various aspects ofMaskProjectTuple
. However, ensure that the assertions cover all relevant properties of theMaskProjectTuple
to fully validate its behavior.- 802-810: The
test_save_base_extension_parse_no_ext
method correctly raises a ValueError when no extensions are provided. This is a good practice for ensuring the robustness ofSaveBase
subclasses.- 822-832: The
test_load_points
method correctly tests the loading functionality ofLoadPoints
. Ensure that the test covers all relevant aspects of the loading process, including error handling and edge cases.- 834-860: The
test_open_tar_file_with_*
methods correctly test theopen_tar_file
utility function with various inputs. Ensure comprehensive coverage for all possible input types and error scenarios.- 885-890: The
TestSaveScreenshot
class correctly defines tests forSaveScreenshot
. Ensure that the tests cover all functionalities and edge cases of theSaveScreenshot
entity.- 901-915: The
TestLoadROIFromTIFF
class provides a basic test forLoadROIFromTIFF
. Consider adding more tests to cover different TIFF formats and error scenarios.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (1)
- package/tests/test_PartSegCore/test_io.py (9 hunks)
Additional comments: 15
package/tests/test_PartSegCore/test_io.py (15)
- 11-11: BytesIO import added without direct usage in the provided code changes.
- 33-33: LoadPoints entity added. Ensure it's properly integrated and tested.
- 35-35: SaveMaskAsTiff entity added. Verify its implementation and integration.
- 37-37: SaveScreenshot entity added. Confirm its usage and test coverage.
- 42-42: open_tar_file utility function added. Check for its correct implementation and usage.
- 48-48: LoadROIFromTIFF entity added. Ensure it's properly implemented and integrated.
- 316-316: Usage of
UPDATE_NAME_JSON
constant intest_update_name
method. Confirm the constant's definition matches expected test data structure.- 822-832:
test_load_points
method added. Verify the correctness of the test, especially the assertions related to the shape ofres.points
.- 802-810:
test_save_base_extension_parse_no_ext
method tests for an exception when no extensions are defined. This is a good practice for ensuring robustness inSaveBase
subclasses.- 812-819:
test_save_base_extension_parse_malformatted_ext
method tests for an exception when extensions are malformatted. This ensures error handling is in place for incorrect subclass definitions.- 834-850:
test_open_tar_file_with_path
and related methods test theopen_tar_file
utility function with various inputs. Ensure these tests cover all expected input types and edge cases.- 882-889:
TestSaveScreenshot
class tests theSaveScreenshot
entity. Verify that the tests cover all functionalities and edge cases of the screenshot saving process.- 898-913:
TestLoadROIFromTIFF
class tests theLoadROIFromTIFF
entity. Ensure the tests adequately cover the loading process and handle different TIFF formats and data structures.- 915-915:
UPDATE_NAME_JSON
constant used intest_update_name
. Confirm the JSON structure is correctly defined and matches the expected format for the test.- 996-1002:
POINTS_DATA
constant added for testingLoadPoints
. Verify the data format and usage in tests align with the expected structure for point data.
assert save_method.get_short_name().lower() == save_method.get_short_name() | ||
assert save_method.get_short_name().isalpha() | ||
assert load_method.get_short_name().lower() == load_method.get_short_name() | ||
assert load_method.get_short_name().isalpha() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions in perform_roi_info_test
method are redundant and do not contribute to the test's purpose.
- assert save_method.get_short_name().lower() == save_method.get_short_name()
- assert save_method.get_short_name().isalpha()
- assert load_method.get_short_name().lower() == load_method.get_short_name()
- assert load_method.get_short_name().isalpha()
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
assert save_method.get_short_name().lower() == save_method.get_short_name() | |
assert save_method.get_short_name().isalpha() | |
assert load_method.get_short_name().lower() == load_method.get_short_name() | |
assert load_method.get_short_name().isalpha() |
def test_load_workflow_from_text(self, bundle_test_dir): | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
data_text = ff.read() | ||
assert isinstance(load_metadata_base(data_text)["workflow"], CalculationPlan) | ||
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | ||
isinstance(load_metadata_base(ff)["workflow"], CalculationPlan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method test_load_workflow_from_text
reads a workflow JSON file twice but only asserts the type of loaded data in one instance. Ensure consistency in testing and consider removing redundant file reading or adding relevant assertions.
- with open(os.path.join(bundle_test_dir, "workflow.json")) as ff:
- isinstance(load_metadata_base(ff)["workflow"], CalculationPlan)
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
def test_load_workflow_from_text(self, bundle_test_dir): | |
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | |
data_text = ff.read() | |
assert isinstance(load_metadata_base(data_text)["workflow"], CalculationPlan) | |
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | |
isinstance(load_metadata_base(ff)["workflow"], CalculationPlan) | |
def test_load_workflow_from_text(self, bundle_test_dir): | |
with open(os.path.join(bundle_test_dir, "workflow.json")) as ff: | |
data_text = ff.read() | |
assert isinstance(load_metadata_base(data_text)["workflow"], CalculationPlan) |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (5)
- package/PartSegCore/analysis/load_functions.py (1 hunks)
- package/PartSegCore/autofit.py (3 hunks)
- package/PartSegCore/io_utils.py (11 hunks)
- package/PartSegCore/napari_plugins/loader.py (1 hunks)
- package/tests/test_PartSegCore/test_napari_plugins.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/PartSegCore/io_utils.py
Additional comments: 3
package/PartSegCore/autofit.py (1)
- 1-1: Importing
warnings
is necessary for the deprecation warning. This change is appropriate.package/tests/test_PartSegCore/test_napari_plugins.py (2)
- 9-9: Importing
ProjectTuple
is necessary for the new test function. This change is appropriate.- 24-24: Renaming the import from
Image
toPImage
avoids naming conflicts and clarifies the code. This change is appropriate.
@@ -95,9 +95,9 @@ def partseg_loader(loader: typing.Type[LoadBase], path: str): | |||
|
|||
try: | |||
project_info = loader.load(load_locations) | |||
except WrongFileTypeException: | |||
except WrongFileTypeException: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of # pragma: no cover
to exclude exception handling from test coverage should be justified. If this exception is unlikely or difficult to trigger in tests, explain why in a comment.
return None | ||
|
||
if isinstance(project_info, (ProjectTuple, MaskProjectTuple)): | ||
return project_to_layers(project_info) | ||
return None | ||
return None # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return statement at the end of the function is excluded from test coverage with # pragma: no cover
. Ensure that this is intentional and justified, as it might indicate untested logic paths in the function.
""" | ||
If 3x3 isometric matrix is not rotation matrix | ||
function transform it into rotation matrix | ||
then calculate rotation axis and angel | ||
:param isometric_matrix: 3x3 np.ndarray with determinant equal 1 or -1 | ||
:return: rotation_matrix, rotation axis, rotation angel | ||
""" | ||
warnings.warn("This function is deprecated", FutureWarning, stacklevel=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a deprecation warning to get_rotation_parameters
is a good practice when phasing out old functionality. Ensure that the deprecation timeline and alternative methods (if any) are communicated to the users.
@@ -78,7 +80,7 @@ | |||
|
|||
if len(voxel_size) != image.ndim: | |||
if len(voxel_size) != len(iter_dim): | |||
raise ValueError("Cannot fit voxel size to array") | |||
raise ValueError("Cannot fit voxel size to array") # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding the exception handling from test coverage with # pragma: no cover
should be justified. If this error condition is unlikely or difficult to simulate in a test environment, consider adding a comment explaining the reasoning.
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") | ||
proj = ProjectTuple(file_path="", image=img) | ||
res = project_to_layers(proj) | ||
assert len(res) == 1 | ||
assert isinstance(res[0][0], np.ndarray) | ||
assert res[0][2] == "labels" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of test_project_to_layers_roi
is a good practice for increasing test coverage, especially for testing ROI layer creation. Ensure that the test covers all relevant cases and edge conditions for the functionality.
@@ -108,6 +120,7 @@ | |||
assert napari_write_labels_tiff(data_path, *layer.as_layer_data_tuple()[:2]) | |||
proj = LoadROIFromTIFF.load([data_path]) | |||
assert proj.roi_info.roi.shape == data.shape | |||
assert napari_write_labels_tiff(str(tmp_path / "test.seg"), *layer.as_layer_data_tuple()[:2]) is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion for testing napari_write_labels_tiff
function in test_save_load_axis_order
is a good addition for ensuring the functionality works as expected. Consider adding more assertions to cover different scenarios and edge cases.
Summary by CodeRabbit