Skip to content
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

refactor: generic hesai unit tests #80

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Oct 2, 2023

PR Type

  • Improvement
  • Refactoring

Related Links

Description

This PR refactors the Hesai unit tests to a generic form.
This is motivated by another PR I will open shortly, which implements a new unit test for each sensor, which I did not want to implement over and over again for each sensor model.

Previously, for each sensor model, a hesai_ros_decoder_test_XYZ.cpp containing a full implementation of the HesaiRosDecoderTest node and a hesai_ros_decoder_test_main_XYZ.cpp handling the launch of said node were implemented.
Looking at the diff between the implementations for e.g. Pandar40P and Pandar64, the only differences are the ROS parameter values (such as model_name, bag_path, etc.):

$ diff hesai_ros_decoder_test_40p.cpp hesai_ros_decoder_test_64.cpp 
1c1
< #include "hesai_ros_decoder_test_40p.hpp"
---
> #include "hesai_ros_decoder_test_64.hpp"
84c84,87
< Status HesaiRosDecoderTest::GetStatus() { return wrapper_status_; }
---
> Status HesaiRosDecoderTest::GetStatus()
> {
>   return wrapper_status_;
> }
103c106
<     this->declare_parameter<std::string>("sensor_model", "Pandar40P");
---
>     this->declare_parameter<std::string>("sensor_model", "Pandar64");
145c148
<       "calibration_file", (calib_dir / "Pandar40P.csv").string(), descriptor);
---
>       "calibration_file", (calib_dir / "Pandar64.csv").string(), descriptor);
166c169
<       "bag_path", (bag_root_dir / "40p" / "1673400149412331409").string(), descriptor);
---
>       "bag_path", (bag_root_dir / "64" / "1673403880599376836").string(), descriptor);
274,275d276
< 
< 

This makes for a very straight-forward refactoring, where the parameters are passed into HesaiRosDecoderTest as a parameter struct, and the test cases being replaced by a parametrized one.

Further, to accomodate for future test cases, the HesaiRosDecoderTest::ReadBag() function has been modified to take a callback function as a parameter which is called on each decoded pointcloud and which can then perform functions specific to a given unit test.

Review Procedure

Compile and run unit tests

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

The sensor-specific duplicates of the ..._test_main and ..._test files
have been refactored into a single generic implementation that takes
only the sensor parameters as input.

The ReadBag function has been refactored such that it takes a callback
function which is called on every point cloud scan.
This allows for easy and concise implementation of new test cases.
@mojomex mojomex requested review from amc-nu and drwnz October 2, 2023 05:55
@mojomex mojomex self-assigned this Oct 2, 2023
@mojomex mojomex changed the title Generic hesai unit tests refactor: generic hesai unit tests Oct 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

Attention: 88 lines in your changes are missing coverage. Please review.

Comparison is base (74cdda4) 9.93% compared to head (d8b1ad6) 31.99%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##            main      #80       +/-   ##
==========================================
+ Coverage   9.93%   31.99%   +22.06%     
==========================================
  Files        118        7      -111     
  Lines      10149      722     -9427     
  Branches    1533      448     -1085     
==========================================
- Hits        1008      231      -777     
+ Misses      8036       94     -7942     
+ Partials    1105      397      -708     
Flag Coverage Δ
differential 31.99% <24.78%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
nebula_tests/hesai/hesai_common.hpp 14.81% <0.00%> (-25.19%) ⬇️
nebula_tests/hesai/hesai_ros_decoder_test_main.cpp 34.09% <34.09%> (ø)
nebula_tests/hesai/hesai_ros_decoder_test.cpp 35.02% <25.00%> (ø)

... and 101 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mojomex mojomex mentioned this pull request Oct 2, 2023
5 tasks
@amc-nu amc-nu merged commit 6ceea8d into tier4:main Oct 17, 2023
6 checks passed
@mojomex mojomex deleted the generic-hesai-unit-tests branch October 17, 2023 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants