Ros2 unit test workflow #471
Conversation
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
WalkthroughThis pull request introduces comprehensive changes to the project's testing infrastructure and workflow. A new GitHub Actions workflow for running unit tests has been added, replacing an existing workflow. The changes span multiple packages, including modifications to CMakeLists.txt files to adjust test configurations, updates to the battery driver node's constructor, and refinements to test utilities. The primary focus is on enhancing the testing process, with a particular emphasis on conditional integration testing and improved test execution methods. Changes
Suggested Reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
Documentation and Community
|
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
…workflow Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
…workflow Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
* Test unit test * Add debuging logs * Change in test deconstructor * test * Test * test * Utest * Add test namespace * console_cohesion- * test * Test * Update run-unit-tests-push.yaml * Simplify * Simplify NodeCreation test * Move system_ros_interface to integration test * Fix * Remove redundant workflow
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
![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
🧹 Nitpick comments (4)
husarion_ugv_battery/test/utils/test_battery_driver_node.hpp (1)
106-106: Consider removing the leading slash in namespace for consistency.
You are instantiatingBatteryDriverNodewith"/test_battery"as the namespace. While correct, consider removing the leading slash if you prefer a relative namespace or if you want consistency with other packages..github/workflows/run-unit-tests.yaml (1)
1-42: Efficient CI configuration with potential for caching.
The new workflow appears solid, providing a matrix strategy for simulation and hardware builds. Consider adding caching (e.g., for built packages or dependencies) to accelerate repeated runs.husarion_ugv_manager/CMakeLists.txt (1)
167-167: Consider generalizing the option description.The current description "Run tests on self hosted runner" is specific to CI environment. Consider using a more general description like "Run integration tests" to match other packages and make it clear that this controls integration test execution regardless of the environment.
- option(TEST_INTEGRATION "Run tests on self hosted runner" OFF) + option(TEST_INTEGRATION "Run integration tests" OFF)husarion_ugv_hardware_interfaces/CMakeLists.txt (1)
227-247: Consider consistent test framework usage.The integration test section mixes
ament_add_gtestandament_add_gmock. While both frameworks are valid, consider usingament_add_gmockconsistently as it's a superset of gtest and provides mocking capabilities that might be useful in future test expansions.For example, these tests could be converted to use gmock:
${PROJECT_NAME}_test_system_ros_interface${PROJECT_NAME}_test_canopen_manager${PROJECT_NAME}_test_roboteq_driverAlso applies to: 249-255, 257-268, 277-343
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/run-unit-tests.yaml(1 hunks).github/workflows/unit-tests.yaml(0 hunks)README.md(2 hunks)husarion_ugv_battery/include/husarion_ugv_battery/battery_driver_node.hpp(1 hunks)husarion_ugv_battery/src/battery_driver_node.cpp(1 hunks)husarion_ugv_battery/test/utils/test_battery_driver_node.hpp(2 hunks)husarion_ugv_diagnostics/CMakeLists.txt(1 hunks)husarion_ugv_hardware_interfaces/CMakeLists.txt(2 hunks)husarion_ugv_manager/CMakeLists.txt(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp(0 hunks)husarion_ugv_manager/test/test_behavior_tree_manager.cpp(1 hunks)husarion_ugv_manager/test/test_behavior_tree_utils.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp
- .github/workflows/unit-tests.yaml
✅ Files skipped from review due to trivial changes (2)
- husarion_ugv_manager/test/test_behavior_tree_utils.cpp
- husarion_ugv_manager/test/test_behavior_tree_manager.cpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (7)
husarion_ugv_battery/include/husarion_ugv_battery/battery_driver_node.hpp (1)
41-42: Empower user-defined namespace with caution.
The new optionalnsparameter forBatteryDriverNodelooks good for configurability. Be mindful, however, that including a leading slash ("/") can occasionally lead to unexpected fully qualified names. Ensure that this usage is consistent with the broader ROS 2 naming conventions across the codebase.husarion_ugv_battery/src/battery_driver_node.cpp (1)
39-41: Constructor signature aligns with ROS 2 best practices.
Initializes the node correctly by passingnode_name,ns, andoptionsto the base class constructor. No issues found.husarion_ugv_battery/test/utils/test_battery_driver_node.hpp (1)
139-146: Robust file I/O error handling appreciated.
The two-step validation (checking if the file was created successfully and if writing was successful) is good. This ensures clear messaging in case of errors.husarion_ugv_diagnostics/CMakeLists.txt (1)
76-76: LGTM! Good separation of integration tests.The addition of the
TEST_INTEGRATIONoption with defaultOFFis a good practice for controlling the execution of integration tests. This aligns with the PR objectives and follows standard CMake practices.husarion_ugv_manager/CMakeLists.txt (1)
168-187: LGTM! Well-organized test structure.The integration tests are properly organized under the TEST_INTEGRATION condition. This separation allows for better control over test execution and aligns with the PR objectives.
husarion_ugv_hardware_interfaces/CMakeLists.txt (1)
224-225: LGTM! Clear separation of integration tests.The integration tests section is well-marked and the TEST_INTEGRATION option is properly configured with default OFF.
README.md (1)
140-152: LGTM! Clear and comprehensive testing instructions.The new sections provide clear instructions for running tests in different environments. The documentation properly explains the usage of the TEST_INTEGRATION flag and aligns well with the code changes.
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/run-unit-tests.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/run-unit-tests.yaml
47-47: property "build_test_1" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (4)
.github/workflows/run-unit-tests.yaml (4)
1-8: LGTM! Well-configured workflow triggers.The workflow triggers are appropriately set up to run on push events while also allowing manual triggering and reuse in other workflows.
9-16: LGTM! Effective use of matrix strategy.The matrix strategy efficiently handles both simulation and hardware builds, with clear job naming for easy identification in CI logs.
17-27: LGTM! Proper environment setup for ROS 2 testing.The environment configuration is well-structured with appropriate action versions and ROS 2 testing enabled.
28-43: Verify the existence of build-specific dependency files.Please ensure that both
simulation_deps.reposandhardware_deps.reposfiles exist in thehusarion_ugvdirectory.✅ Verification successful
Build dependency files are correctly configured
Both
simulation_deps.reposandhardware_deps.reposfiles exist in thehusarion_ugvdirectory, confirming that the workflow configuration is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if both deps files exist ls -l husarion_ugv/simulation_deps.repos husarion_ugv/hardware_deps.reposLength of output: 241
| } | ||
| } | ||
|
|
||
| continue-on-error: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider using continue-on-error: true.
Using continue-on-error: true might hide build or test failures. Consider removing it since you already have a retry mechanism in place.
- continue-on-error: true| - name: Retry Build and test (if failed) | ||
| if: steps.build_test_1.outcome == 'failure' | ||
| uses: ros-tooling/action-ros-ci@v0.3 | ||
| with: | ||
| vcs-repo-file-url: ${{ github.workspace }}/husarion_ugv/${{ env.HUSARION_ROS_BUILD_TYPE }}_deps.repos | ||
| target-ros2-distro: humble | ||
| colcon-defaults: | | ||
| { | ||
| "build": { | ||
| "packages-up-to": ["husarion_ugv"], | ||
| "cmake-args": ["-DCMAKE_BUILD_TYPE=Release"] | ||
| }, | ||
| "test": { | ||
| "packages-up-to": ["husarion_ugv"] | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the retry condition step ID reference.
The condition references an undefined step ID build_test_1. The previous step needs an ID to be referenced in the retry condition.
Apply this diff to fix the step ID reference:
- name: Build and test
+ id: build_test_1
uses: ros-tooling/action-ros-ci@v0.3Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
47-47: property "build_test_1" is not defined in object type {}
(expression)
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Description
TEST_INTEGRATIONRequirements
Tests 🧪
Summary by CodeRabbit
Release Notes
New Features
Documentation
Testing
Chores