chore: Housekeeping + improved test coverage #24
Conversation
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
While this will not benifit the normal usecase this reduced pytest from 12 sec to 3 sec
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on housekeeping improvements and significantly expands test coverage across multiple modules. It addresses technical debt by refactoring the project to make ROS1 support optional and removing the hard dependency on scipy for rotation calculations.
- Introduced optional ROS1 support with proper import guards and runtime error handling
- Replaced scipy dependency with custom rotation utilities for quaternion and point cloud operations
- Added comprehensive test coverage for timing, tf operations, QoS handling, rotation calculations, and plugin functionality
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_timing.py | New comprehensive test suite for ROS1 time fixing functionality |
| test/test_tf.py | Added test for tf_remove functionality with 'all' option |
| test/test_settings.py | New test for SettingRotation validation |
| test/test_rotation.py | Extensive test suite comparing custom rotation implementation with scipy |
| test/test_qos.py | Complete test coverage for QoS parsing and serialization |
| test/test_plugin.py | Added test for multiple plugin implementations |
| test/test_msg_def.py | Tests for message definition handling and URL validation |
| test/test_main.py | Added version command test |
| test/test_json_to_mcap.py | Fixed regex pattern for error message matching |
| test/test_camera_info.py | Test for error handling when header attribute is missing |
| test/e2e/convert/transforms/* | End-to-end test files for tf_remove_all functionality |
| test/e2e/convert/transforms/tf_offset_combined.expected.jsonl | Updated expected values for quaternion precision |
| test/e2e/convert/pointcloud/rotate_quaternion.expected.jsonl | Updated expected rotation results |
| src/kappe/writer.py | Added optional ROS1 support with import guards |
| src/kappe/utils/settings.py | Replaced scipy dependency with custom euler_to_quaternion |
| src/kappe/utils/rotation.py | New module with custom rotation utilities |
| src/kappe/utils/msg_def.py | Performance improvements with LRU cache and tuple optimization |
| src/kappe/utils/mcap_to_json.py | Moved import to top level |
| src/kappe/utils/json_to_mcap.py | Moved import to top level |
| src/kappe/plugins/image.py | Removed unused SaveCompress class and imports |
| src/kappe/plugin.py | Simplified plugin loading logic |
| src/kappe/module/timing.py | Added optional ROS1 support and fixed recursion bug |
| src/kappe/module/tf.py | Replaced scipy with custom quaternion multiplication |
| src/kappe/module/pointcloud.py | Replaced scipy with custom rotation utilities |
| pyproject.toml | Restructured dependencies to make ROS1 optional |
| .pre-commit-config.yaml | Updated tool versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
siliconlad
approved these changes
Dec 2, 2025
Collaborator
siliconlad
left a comment
siliconlad
left a comment
There was a problem hiding this comment.
Small things, otherwise looks good!
Comment on lines
-83
to
-94
|
|
||
|
|
||
| class SaveCompress(ConverterPlugin): | ||
| def __init__(self, *, quality: int = 10) -> None: | ||
| self.quality = quality | ||
| self.counter = 0 | ||
|
|
||
| def convert(self, ros_msg: Any) -> Any: | ||
| stream = BytesIO(ros_msg.data) | ||
| img = Image.open(stream) | ||
| with Path(f'{self.counter:08}.jpeg').open('wb') as f: | ||
| img.save(f, format='jpeg', quality=self.quality) |
| assert 'string data' in result | ||
|
|
||
|
|
||
| def test_get_message_definition_with_custom_folder(tmp_path: Path): |
Collaborator
There was a problem hiding this comment.
Custom + std_msgs could be another test.
i.e.
msg_file.write_text('std_msgs/Header header\nstring name\n')
Comment on lines
+17
to
+22
| yaml_str = """ | ||
| - history: 1 | ||
| depth: 10 | ||
| reliability: 2 | ||
| durability: 2 | ||
| """ |
Collaborator
There was a problem hiding this comment.
My favourite method from textwrap.
Suggested change
| yaml_str = """ | |
| - history: 1 | |
| depth: 10 | |
| reliability: 2 | |
| durability: 2 | |
| """ | |
| yaml_str = dedent(""" | |
| - history: 1 | |
| depth: 10 | |
| reliability: 2 | |
| durability: 2 | |
| """) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.