Add dependency injection example using path generator #23
Conversation
| }; | ||
|
|
||
| struct PathGenerator { | ||
| struct MiddlewareHandle { |
There was a problem hiding this comment.
The reason that this is public is that this middlewarehandle struct is specific for the PathGenerator class right? If I wanted one for another class, like MissionControl or something, I'm gonna have to make the MiddlewareHandle again in there?
There was a problem hiding this comment.
Correct. Though, to be clear:
- It is public so that the ros middleware and mock middleware can inherit from it
- It is declared in the
PathGeneratorclass because I like that style to say that it is specific to this class. Using a namespace would be equally as valid (or just making a longer name) - If a generic version would work, say by implementing all of the components to abstract and then wrapping those up in a parameter object would probably also work
| * @brief Builds paths given a map | ||
| * @param mw The middleware handle for interacting with services | ||
| */ | ||
| PathGenerator(std::unique_ptr<MiddlewareHandle> mw) : mw_{std::move(mw)} { |
There was a problem hiding this comment.
The move is to ensure that PathGenerator is responsible for the lifetime of the middleware handle right? The middleware object in the original scope will now cause UB because it's been moved?
There was a problem hiding this comment.
If the middleware object in the original scope is accessed it will be UB... or it will throw, not sure.
The std::move is required because it's a unique_ptr. I don't think it will compile otherwise.
| if (map_.get_data().size() == 0) { | ||
| mw_->log_error("MAP IS EMPTY!!"); | ||
| response->success.data = false; | ||
| response->path = std_msgs::msg::UInt8MultiArray(); | ||
| return; | ||
| } | ||
| // Check to make sure start and goal fields of the request are of | ||
| // size 2 | ||
| if (request->start.data.size() != 2) { | ||
| mw_->log_error("START POSITION MUST CONTAIN TWO ELEMENTS!!"); | ||
| response->success.data = false; | ||
| response->path = std_msgs::msg::UInt8MultiArray(); | ||
| return; | ||
| } | ||
| if (request->goal.data.size() != 2) { | ||
| mw_->log_error("GOAL POSITION MUST CONTAIN TWO ELEMENTS!!"); | ||
| response->success.data = false; | ||
| response->path = std_msgs::msg::UInt8MultiArray(); | ||
| return; | ||
| } | ||
| auto const start = | ||
| Position{request->start.data[0], request->start.data[1]}; | ||
| auto const goal = | ||
| Position{request->goal.data[0], request->goal.data[1]}; | ||
|
|
||
| // Generate the path | ||
| auto const path = generate_global_path(start, goal); | ||
|
|
||
| // Start populating the response message | ||
| auto response_path = std_msgs::msg::UInt8MultiArray(); | ||
|
|
||
| if (path.has_value()) { | ||
| response_path.layout.dim.resize( | ||
| 3, std_msgs::msg::MultiArrayDimension()); | ||
|
|
||
| response_path.layout.dim[0].label = "rows"; | ||
| response_path.layout.dim[0].size = path.value().size(); | ||
| response_path.layout.dim[0].stride = path.value().size() * 2; | ||
|
|
||
| response_path.layout.dim[1].label = "columns"; | ||
| response_path.layout.dim[1].size = 2; | ||
| response_path.layout.dim[1].stride = 1; | ||
|
|
||
| response_path.layout.dim[2].label = "channel"; | ||
| response_path.layout.dim[2].size = 1; | ||
| response_path.layout.dim[2].stride = 1; | ||
|
|
||
| // Start pushing back the path only if there is one | ||
| if (path.value().size() > 0) { | ||
| for (auto const& position : path.value()) { | ||
| response_path.data.push_back(position.x); | ||
| response_path.data.push_back(position.y); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| response->success.data = path.has_value(); | ||
| response->path = response_path; | ||
| }); |
There was a problem hiding this comment.
Should all of the guts be moved into it's own function for the sake of readability or does that defeat the purpose?
There was a problem hiding this comment.
As I think we've discussed now, moving it in to it's own function would be just fine. I didn't do it here because it just wasn't what I was highlighting.
| mw_->log_error("MAP IS EMPTY!!"); | ||
| response->success.data = false; | ||
| response->path = std_msgs::msg::UInt8MultiArray(); | ||
| return; |
There was a problem hiding this comment.
This is where you added the return right?
| * @param goal The goal position | ||
| * @return The path from start to goal if it exists, std::nullopt otherwise | ||
| */ | ||
| std::optional<Path> generate_global_path( |
There was a problem hiding this comment.
Should the return type be Path or std::optional<Path>? Does it defeat the purpose to have it be optional?
There was a problem hiding this comment.
If the code can fail to produce an output, returning a std::optional is a great way to communicate that, at least until we have std::expected
| TEST(PathGenerator, SetMap) { | ||
| // GIVEN a path generator | ||
| auto mw = std::make_unique<MockMiddlewareHandle>(); | ||
| // Capture the callback so it can be called later | ||
| PathGenerator::MiddlewareHandle::SetMapServiceCallback callback; | ||
| ON_CALL(*mw, register_set_map_service(testing::_)) | ||
| .WillByDefault(testing::SaveArg<0>(&callback)); | ||
|
|
||
| auto const path_generator = PathGenerator{std::move(mw)}; | ||
|
|
||
| // WHEN the set map service is called | ||
| auto const request = make_costmap(); | ||
| auto response = std::make_shared<example_srvs::srv::SetMap::Response>(); | ||
| callback(request, response); | ||
|
|
||
| // THEN the path generator should successfully set the map | ||
| EXPECT_TRUE(response->success.data); | ||
| } |
There was a problem hiding this comment.
Here, we have callback, which is the std::function type for the set map service callback, and when register_set_map_service is called by PathGenerator, save the callback that the PathGenerator registers, and then test the callback?
There was a problem hiding this comment.
Exactly. Perfect.
| TEST(PathGenerator, NoCostmap) { | ||
| // GIVEN a path generator with a costmap | ||
| auto mw = std::make_unique<MockMiddlewareHandle>(); | ||
| // Capture the path callback so it can be called later | ||
| PathGenerator::MiddlewareHandle::GeneratePathServiceCallback path_callback; | ||
| ON_CALL(*mw, register_generate_path_service(testing::_)) | ||
| .WillByDefault(testing::SaveArg<0>(&path_callback)); | ||
|
|
||
| auto const path_generator = PathGenerator{std::move(mw)}; | ||
|
|
||
| // WHEN the generate path service is called without a costmap | ||
| auto path_request = std::make_shared<example_srvs::srv::GetPath::Request>(); | ||
| path_request->start.data = {0, 0}; | ||
| path_request->goal.data = {0, 0}; | ||
|
|
||
| auto path_response = std::make_shared<example_srvs::srv::GetPath::Response>(); | ||
| path_callback(path_request, path_response); | ||
|
|
||
| // THEN the path generator should fail | ||
| EXPECT_FALSE(path_response->success.data); | ||
| } |
There was a problem hiding this comment.
Same as above, but for the get path service, right?
| ON_CALL(*mw_, register_set_map_service(testing::_)) | ||
| .WillByDefault([&](auto const& map_callback) { | ||
| auto const map_request = make_costmap(); | ||
| auto map_response = | ||
| std::make_shared<example_srvs::srv::SetMap::Response>(); | ||
| map_callback(map_request, map_response); | ||
| }); |
There was a problem hiding this comment.
Does this mean that when register_set_map_service is called by in the PathGenerator constructor, the argument to the register_set_map_service is input to the lambda here? In this case, map_callback, is the lambda on line 108, and this lambda uses that map_callback to set the costmap inside of the PathGenerator?
| // GIVEN a path generator with a costmap | ||
| auto const path_generator = PathGenerator{std::move(mw_)}; | ||
|
|
||
| // WHEN the generate path service is called, | ||
| auto const path_request = | ||
| std::make_shared<example_srvs::srv::GetPath::Request>(); | ||
| auto path_response = std::make_shared<example_srvs::srv::GetPath::Response>(); | ||
| path_callback_(path_request, path_response); | ||
|
|
||
| // THEN the path generator should fail | ||
| EXPECT_FALSE(path_response->success.data); |
There was a problem hiding this comment.
So now we have a PathGenerator whose costmap is set due to the stuff in the PathGeneratorFixture constructor. Then we test the get path callback.
| TEST_F(PathGeneratorFixture, SameStartGoal) { | ||
| // GIVEN a path generator with a costmap | ||
| auto const path_generator = PathGenerator{std::move(mw_)}; | ||
|
|
||
| // WHEN the generate path service is called with the same start and goal | ||
| auto path_request = std::make_shared<example_srvs::srv::GetPath::Request>(); | ||
| path_request->start.data = {0, 0}; | ||
| path_request->goal.data = {0, 0}; | ||
| auto path_response = std::make_shared<example_srvs::srv::GetPath::Response>(); | ||
| path_callback_(path_request, path_response); | ||
|
|
||
| // THEN the path generator should succeed | ||
| EXPECT_TRUE(path_response->success.data); | ||
| auto const expected = std::vector<Position>{{0, 0}}; | ||
| // AND the path should be the same as the start | ||
| EXPECT_EQ(parseGeneratedPath(path_response->path), expected); | ||
| } |
There was a problem hiding this comment.
Same as above, but now its a different test case.
Description
Adds gtest which refactored
PathGeneratorto use dependency injectionTesting
Using the container
Notes
Please add any notes to help reviewers understand how this code should be reviewed.
Be sure to not only request reviewers but also assign someone to the pull request so it is clear when a review is required.