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

Adds a basic unit test for PointCloud functionality #496

Merged
merged 13 commits into from
Oct 13, 2023

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Oct 26, 2022

This PR adds a very trivial (happy path) unit test for PointCloud functionality. Hopefully, I will get more time to add more functionality to this test over time.

TODOS:

  • Check behaviour with NaNs
  • Check mismatched float and pointcloud sizes
  • Check malformed point cloud.

Signed-off-by: Arjo Chakravarty [email protected]

This PR adds a very trivial (happy path) unit test for PointCloud functionality. Hopefully, I will get more time to add more functionality to this test over time.

TODOS:
* Check behaviour with NaNs
* Check mismatched float and pointcloud sizes
* Check malformed point cloud.

Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Oct 26, 2022
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #496 (69b3972) into gz-gui7 (c93dd14) will increase coverage by 2.12%.
The diff coverage is n/a.

❗ Current head 69b3972 differs from pull request most recent head e79ec8d. Consider uploading reports for the commit e79ec8d to get more accurate results

@@             Coverage Diff             @@
##           gz-gui7     #496      +/-   ##
===========================================
+ Coverage    70.91%   73.04%   +2.12%     
===========================================
  Files           45       45              
  Lines         4941     4941              
===========================================
+ Hits          3504     3609     +105     
+ Misses        1437     1332     -105     

see 1 file with indirect coverage changes

test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 requested a review from ahcorde October 26, 2022 11:13
@jennuine
Copy link
Contributor

@arjo129 what is the status of this PR? Will you be addressing the TODOs here?

@arjo129
Copy link
Contributor Author

arjo129 commented Nov 23, 2022

We can merge this as is and Ill increase coverage over time.

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this test be added to PointCloud_TEST instead?

test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
test/integration/point_cloud.cc Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some more minor comments. Could you summarize the reason for the point cloud publisher/msg? In the marker message callback, only the flatMsg data is being checked but not pcMsg. It would be nice to check the point cloud data as well.

src/plugins/point_cloud/PointCloud_TEST.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Show resolved Hide resolved
src/plugins/point_cloud/PointCloud_TEST.cc Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Copy link
Contributor Author

arjo129 commented Dec 22, 2022

Could you summarize the reason for the point cloud publisher/msg? In the marker message callback, only the flatMsg data is being checked but not pcMsg. It would be nice to check the point cloud data as well.

The rationale for this is that the pcMsg provides data about position of the markers whereas the marker messages provide data about position. The test checks that the marker messages have the correct color values (which is what the point-cloud plugin basically does).

@jennuine jennuine self-requested a review April 10, 2023 18:57
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 18, 2023
Signed-off-by: Ian Chen <[email protected]>
@mjcarroll
Copy link
Contributor

I think that the focal failures here in CI are unrelated to this PR?

@arjo129 arjo129 merged commit 0c1cb7d into gz-gui7 Oct 13, 2023
13 checks passed
@arjo129 arjo129 deleted the arjo/test/pointcloud branch October 13, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants