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.

Sign up for GitHub

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

Jump to bottom

Porting more tests to tf2_ros #202

Merged
merged 12 commits into from
Jan 10, 2020
Merged

Porting more tests to tf2_ros #202

merged 12 commits into from
Jan 10, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Dec 4, 2019

Porting more tests to tf2_ros

  • listener_unittest
  • time_reset_test

clalancette
clalancette requested changes Dec 4, 2019
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

First, good work on porting more tests! That's awesome.

I've got a couple of things that I think should be fixed in here, then we can run CI on it.

tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
@ahcorde ahcorde added the enhancement New feature or request label Dec 17, 2019
clalancette
clalancette reviewed Dec 19, 2019
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few more comments. Most of them are minor, so once those are fixed I would suggest running CI.

tf2_ros/test/listener_unittest.cpp Outdated
Comment on lines 63 to 64
double x, y, z;
generate_rand_numbers(x, y, z);
Copy link
Contributor

Choose a reason for hiding this comment

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

Honest question: is it any better to generate random numbers rather than just hardcode some values for x, y, and z?

tf2_ros/test/listener_unittest.cpp Show resolved Hide resolved
tf2_ros/test/test_buffer_client.cpp Outdated
std::bind(&MockBufferServer::handle_accepted, this, std::placeholders::_1));
}

void handle_accepted(const std::shared_ptr<GoalHandle> goal_handle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: all of the other methods in here are camelCase, I think this should be handleAccepted.

tf2_ros/test/time_reset_test.cpp Outdated
{
rclcpp::Rate r(10);
rclcpp::spin_some(node);
for (uint8_t i = 0; i < 10; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

This really doesn't matter at all here, but for future reference an int loop counter will be more efficient than a uint8_t.

tf2_ros/test/time_reset_test.cpp Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 19, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 20, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

clalancette
clalancette requested changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

There's one syntax error in here. There also seem to be some failing tests in CI that need to be fixed. Once those are fixed, this looks good to go.

tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 7, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 7, 2020

I have commented the test_buffer_client.py because for some reason is not working in the CI. This error is happening because is not able to find the action server but the mock should created it

clalancette
clalancette reviewed Jan 10, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've got two more small things to change, then I think we can go forward with this. I'll approve once those are fixed.

Can you please open an issue to track the test_buffer_client.py failures?

tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
tf2_ros/test/time_reset_test.cpp Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 10, 2020

Issue created #212

clalancette
clalancette approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Issue created #212

Thanks! Looks good to me now, thanks for iterating. Before these last two changes, it looks like you had green CI, so I'll just suggest you re-run the tests locally and ensure they all pass. With that, I think we are good to merge.

@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 10, 2020

Thanks for the review @clalancette !

@ahcorde ahcorde merged commit 9940f10 into ros2 Jan 10, 2020
@ahcorde ahcorde deleted the ahcorde/test/tf2_ros branch January 10, 2020 16:03
@ahcorde ahcorde mentioned this pull request Jan 23, 2020
Merged
ahcorde added a commit that referenced this pull request Jan 23, 2020
@ahcorde
Porting more tests to tf2_ros (#202)
c1169c5 
* Added more tests to tf2_ros

* improving tf2_ros time_reset_test

* tf2_ros fixed failed test_buffer_client.cpp

* added some EXPECT to listener unittest

* reviews

* Update listener_unittest.cpp

* fixed tf2_ros time_reset_test

* tf2_ros removed ROS launch files

* Added TODO to fix test_buffer_client in CI

* tf2_ros added feedback
ahcorde added a commit that referenced this pull request Jan 24, 2020
@ahcorde
Porting more tests to tf2_ros (#202) (#225)
fa74794 
* Added more tests to tf2_ros

* improving tf2_ros time_reset_test

* tf2_ros fixed failed test_buffer_client.cpp

* added some EXPECT to listener unittest

* reviews

* Update listener_unittest.cpp

* fixed tf2_ros time_reset_test

* tf2_ros removed ROS launch files

* Added TODO to fix test_buffer_client in CI

* tf2_ros added feedback
@dirk-thomas dirk-thomas mentioned this pull request Aug 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@clalancette clalancette clalancette approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ahcorde @clalancette @dirk-thomas