Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Replace IMU messages with new dummy messages #121

Merged
merged 3 commits into from Apr 2, 2020

Conversation

prajakta-gokhale
Copy link
Member

  • Define a new message type DummyMessage that has a Header
  • Use DummyMessage in unit tests instead of sensor_msgs::msg::Imu
  • Remove dependency on sensor_msgs

Fixes #120.

Signed-off-by: Prajakta Gokhale prajaktg@amazon.com

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #121 into master will increase coverage by 1.84%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   35.06%   36.91%   +1.84%     
==========================================
  Files          29       32       +3     
  Lines        1363     1406      +43     
  Branches      878      877       -1     
==========================================
+ Hits          478      519      +41     
- Misses         81       85       +4     
+ Partials      804      802       -2     
Flag Coverage Δ
#unittests 36.91% <33.33%> (+1.84%) ⬆️
Impacted Files Coverage Δ
...ics_collector/test_subscriber_topic_statistics.cpp 35.89% <33.33%> (+0.04%) ⬆️
...r/msg/dds_fastrtps/dummy_message__type_support.cpp 94.73% <0.00%> (ø)
...rics_collector/msg/dummy_message__type_support.cpp 60.00% <0.00%> (ø)
...em_metrics_collector/msg/dummy_message__struct.hpp 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aae3cb5...d5dbaa4. Read the comment docs.

@prajakta-gokhale prajakta-gokhale marked this pull request as ready for review April 1, 2020 03:46
Copy link
Member

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

Exposing publicly the test message now forces us to release it, and preserving some type of compatibility as it's a "publicly accessible resource". Why not just put the message in test/dummy.msg, and drop the new package altogether?

Copy link
Member

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

Actually, I thought it was a new package, but it's dropped in the middle of messages we use in prod. Let's not do that.

@prajakta-gokhale
Copy link
Member Author

Why not just put the message in test/dummy.msg, and drop the new package altogether?

I'll try that. Since it's needed in 2 packages (libstatictics_collector and topic_statisticvs_collector), is it okay to depend on test/dummy.msg of one package from another?

@thomas-moulard
Copy link
Member

I think duplicating the test message is OK as this is trivial.

@prajakta-gokhale
Copy link
Member Author

As I understand, in order to move DummyMessage to individual packages that need them, these library changes had to be made:

  • Create two separate target libraries ${PROJECT_NAME} and ${PROJECT_NAME}_shared. The message generation step creates a Utility library with the name ${PROJECT_NAME}. This has to be kept separate from the Shared library that the package exports.
  • The Utility library name has to be same as ${PROJECT_NAME} because all the typesupport_impl's are named package_name__typesupport__rmw_name by rosidl generators.

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
@prajakta-gokhale prajakta-gokhale merged commit 090da35 into master Apr 2, 2020
@prajakta-gokhale prajakta-gokhale deleted the prajaktg/dummy-msgs branch April 2, 2020 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a dummy message instead of sensor_msgs in tests and demos
3 participants