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

Fix LoadFromBag assumptions causing C++ exceptions during serialization #438

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Feb 16, 2024

Description

  • Not all bags have only GridMap messages
  • Not all bags have GridMap on the right topic
  • Add test for trying to load a grid map from a bag that doesn't contain it on the expected topic
  • Add nullptr check on reader messages

Demo

image

Details

I uploaded a super small bagfile of two chatter topics. It's very small:

ryan@B650-970:~/Dev/ros2_ws/src/grid_map/grid_map_ros$ ll -h resource/double_chatter/
total 56K
drwxrwxr-x 2 ryan ryan 4.0K Feb 16 11:48 ./
drwxrwxr-x 3 ryan ryan 4.0K Feb 16 11:49 ../
-rw-r--r-- 1 ryan ryan  40K Feb 16 00:50 double_chatter.db3
-rw-rw-r-- 1 ryan ryan 4.8K Feb 16 11:48 metadata.yaml

It might be better to construct the bag file on the fly in the test case, but that would take much more time than what I did here.

Issue

Solves #401

@Ryanf55 Ryanf55 changed the title Fix LoadFromBag assumptions Fix LoadFromBag assumptions causing C++ exceptions during serialization Feb 16, 2024
@Ryanf55 Ryanf55 force-pushed the 401-fix-load-from-bag-assumptions-about-message-types branch from 978e073 to 44cda3b Compare February 16, 2024 19:04
@Ryanf55 Ryanf55 added bug ros2 Affects ROS 2 labels Feb 16, 2024
@Ryanf55 Ryanf55 self-assigned this Feb 16, 2024
@Ryanf55 Ryanf55 added this to the 2.2.0 milestone Feb 16, 2024
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Feb 16, 2024

Hello @afrixs, can I get a review and approval is this solves your issue?

@afrixs
Copy link

afrixs commented Feb 16, 2024

Hello @Ryanf55, thank you for looking into this! I'll review and test it on Monday

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Feb 16, 2024

Hello @Ryanf55, thank you for looking into this! I'll review and test it on Monday

Thanks. If you have any bag files you can share to reproduce your failure, it would be much appreciated. I could cut them down and add a specific test for it.

@afrixs
Copy link

afrixs commented Feb 20, 2024

Hello,
the fix looks good, I'm sending a minimalistic bag file which contains an empty grid map + chatter topic (unfortunately I cannot post the actual bag where I found the problem, since it contains map of a private property of a third person), along with tests to check for a valid and invalid requested topic. Note that it is a good practice to add a succeeding test as well to check that the test is well-written (in your case the chatter1 topic was missing / at the beginning, so it would fail even if there was a GridMap message)

test_multitopic.zip

TEST(RosbagHandling, checkTopicTypes)
{
  // This test validates loadFromBag will reject a topic of the wrong type or
  // non-existing topic and accept a correct topic.

  std::string pathToBag = "test_multitopic";
  string topic_wrong = "/chatter";
  string topic_correct = "/grid_map";
  string topic_non_existing = "/grid_map_non_existing";
  GridMap gridMapOut;
  rcpputils::fs::path dir(pathToBag);

  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_wrong, gridMapOut));
  EXPECT_TRUE(GridMapRosConverter::loadFromBag(pathToBag, topic_correct, gridMapOut));
  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_non_existing, gridMapOut));
}

Also, there are still some linting/uncrustify errors to fix in your code

 1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:724:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:727:  Missing space around colon in range-based for loop  [whitespace/forcolon] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:735:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:746:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

 5: @@ -727 +727 @@
5: -  for (const auto& m: topic_metadata) {
5: +  for (const auto & m: topic_metadata) {
5: @@ -735 +735,2 @@
5: -        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'", topic.c_str());
5: +        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'",
5: +      topic.c_str());
5: @@ -746 +747 @@
5: -    // https://github.com/ANYbotics/grid_map/issues/401 
5: +    // https://github.com/ANYbotics/grid_map/issues/401

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages

Signed-off-by: Ryan Friedman <[email protected]>
Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 force-pushed the 401-fix-load-from-bag-assumptions-about-message-types branch from 99d8a40 to 204d490 Compare July 19, 2024 14:27
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jul 25, 2024

Hello, the fix looks good, I'm sending a minimalistic bag file which contains an empty grid map + chatter topic (unfortunately I cannot post the actual bag where I found the problem, since it contains map of a private property of a third person), along with tests to check for a valid and invalid requested topic. Note that it is a good practice to add a succeeding test as well to check that the test is well-written (in your case the chatter1 topic was missing / at the beginning, so it would fail even if there was a GridMap message)

test_multitopic.zip

TEST(RosbagHandling, checkTopicTypes)
{
  // This test validates loadFromBag will reject a topic of the wrong type or
  // non-existing topic and accept a correct topic.

  std::string pathToBag = "test_multitopic";
  string topic_wrong = "/chatter";
  string topic_correct = "/grid_map";
  string topic_non_existing = "/grid_map_non_existing";
  GridMap gridMapOut;
  rcpputils::fs::path dir(pathToBag);

  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_wrong, gridMapOut));
  EXPECT_TRUE(GridMapRosConverter::loadFromBag(pathToBag, topic_correct, gridMapOut));
  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_non_existing, gridMapOut));
}

Also, there are still some linting/uncrustify errors to fix in your code

 1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:724:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:727:  Missing space around colon in range-based for loop  [whitespace/forcolon] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:735:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:746:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

 5: @@ -727 +727 @@
5: -  for (const auto& m: topic_metadata) {
5: +  for (const auto & m: topic_metadata) {
5: @@ -735 +735,2 @@
5: -        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'", topic.c_str());
5: +        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'",
5: +      topic.c_str());
5: @@ -746 +747 @@
5: -    // https://github.com/ANYbotics/grid_map/issues/401 
5: +    // https://github.com/ANYbotics/grid_map/issues/401

I actually tested this code execution through GDB. Because there is only a single bool return from that function, I don't see a way we can test each case without refactoring and adding unit tests for each check. Thoughts?

@Ryanf55 Ryanf55 requested a review from wep21 July 25, 2024 14:24
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jul 28, 2024

I've fixed the tests. They pass locally for me, but not in CI. I don't have time to chase it down right now, but as soon as CI is green, I'm happy to merge this.
https://github.com/ANYbotics/grid_map/actions/runs/10133926333/job/28019598750?pr=438#step:7:2853

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 merged commit 25a1ea5 into ANYbotics:rolling Jul 28, 2024
2 checks passed
@Ryanf55 Ryanf55 deleted the 401-fix-load-from-bag-assumptions-about-message-types branch July 28, 2024 22:58
wep21 pushed a commit that referenced this pull request Jul 29, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jul 29, 2024

@Mergifyio backport jazzy

Copy link

mergify bot commented Jul 29, 2024

backport jazzy

✅ Backports have been created

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jul 29, 2024

@Mergifyio backport iron

mergify bot pushed a commit that referenced this pull request Jul 29, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)
Copy link

mergify bot commented Jul 29, 2024

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 29, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Jul 29, 2024

@Mergifyio backport humble

Copy link

mergify bot commented Jul 29, 2024

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 29, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)
Ryanf55 added a commit that referenced this pull request Jul 31, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)
Ryanf55 added a commit that referenced this pull request Jul 31, 2024
…on (#438) (#468)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)

Co-authored-by: Ryan <[email protected]>
Ryanf55 added a commit that referenced this pull request Jul 31, 2024
…on (#438)

Fix LoadFromBag assumptions

* Not all bags have only GridMap messages
* Not all bags have GridMap on the right topic
* Add test for trying to load a grid map from a bag that doesn't
  contain it on the expected topic
* Add nullptr check on reader messages
* Remove unused include
* Don't skip reporting when tests fail

(cherry picked from commit 25a1ea5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants