Feature/unit tests/simple sensor simulator #27
Conversation
Signed-off-by: yamacir-kit <[email protected]>
Signed-off-by: yamacir-kit <[email protected]>
Signed-off-by: yamacir-kit <[email protected]>
…lt-injection-action
…lt-injection-action
…ion_action/qos_depth fix(action): fix fault injection action - qos
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action
…lt-injection-action Signed-off-by: yamacir-kit <[email protected]>
| { | ||
| auto request = simulation_api_schema::InitializeRequest(); | ||
| request.set_lanelet2_map_path( | ||
| ament_index_cpp::get_package_share_directory("traffic_simulator") + "/map/lanelet2_map.osm"); |
There was a problem hiding this comment.
To avoid dependency on the traffic_simulator package, consider adding the map file lanelet2_map.osm to the simple_sensor_simulator package. This way, the tests won't break if there are changes in the traffic_simulator package.
| #include <simple_sensor_simulator/sensor_simulation/occupancy_grid/grid_traversal.hpp> | ||
| #include <simple_sensor_simulator/sensor_simulation/occupancy_grid/occupancy_grid_builder.hpp> | ||
|
|
||
| auto printGrid(const simple_sensor_simulator::OccupancyGridBuilder & builder) -> void |
There was a problem hiding this comment.
It looks like this function is not currently being used. Perhaps it was helpful during the creation of tests, but if it's no longer needed, we might consider removing it.
| { | ||
| const auto & grid = builder.get(); | ||
| for (size_t i = static_cast<size_t>(0); i < builder.height; ++i) { | ||
| for (size_t j = static_cast<size_t>(0); j < builder.width; ++j) { |
There was a problem hiding this comment.
In my opinion, using 0 directly is completely safe in this specific case. When you have a literal that is zero, it is safe to omit the static_cast, which can improve readability.
However, in the tests below, the use of static_cast is appropriate because it clearly describes the intent and ensures safe type conversion.
| 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 2, 2, 2, 2, 2, 2, 1, 1, 1, | ||
| 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| // clang-format on |
There was a problem hiding this comment.
moving results_expected to the beginning of the test could enhance clarity. Similarly, you might consider applying this approach (Arrange-Act-Assert pattern) to the tests below as well.
| return request; | ||
| } | ||
|
|
||
| class ScenarioSimulatorTest : public testing::Test |
There was a problem hiding this comment.
In my opinion, it is a good practice to separate implementation from headers. But if it passed the review at Tier4, then it's okay
| auto makeAttachLidarSensorRequest() -> simulation_api_schema::AttachLidarSensorRequest | ||
| { | ||
| auto request = simulation_api_schema::AttachLidarSensorRequest(); | ||
| auto configuration = traffic_simulator::helper::constructLidarConfiguration( |
There was a problem hiding this comment.
It looks like the functions makeAttachDetectionSensorRequest and makeAttachLidarSensorRequest are using helper functions from the traffic_simulator package to construct configurations. This dependency could cause tests in the scenario_simulator to fail if there are changes in the traffic_simulator package, rather than changes in the simulation_api_schema.pb.h.
Given that only two helper functions are involved, it seems reasonable to duplicate these functions within the simple_sensor_simulator package. This would ensure that the tests fail solely due to changes in the simulation_api_schema.pb.h without being affected by modifications in the traffic_simulator package.
There was a problem hiding this comment.
We can solve this similarly to how we handle makeAttachOccupancyGridSensorRequest by defining the configuration directly.
There was a problem hiding this comment.
I propose to add new directory where we can put a file with definition of constructLidarConfiguration and other useful functions. Take a look here PR
| EXPECT_TRUE(client.call(makeUpdateFrameRequest()).result().success()); | ||
|
|
||
| rclcpp::shutdown(); | ||
| server.join(); |
There was a problem hiding this comment.
consider moving these lines to destructor or TearDown(). And try to apply this to tests below
Signed-off-by: Masaya Kataoka <[email protected]>
Signed-off-by: Masaya Kataoka <[email protected]>
Signed-off-by: Masaya Kataoka <[email protected]>
Fix/remove arm build test
…ature/clear_route_api
…ct_entity Feature/unit tests/misc object entity
…eous Feature/unit tests/miscellaneous
add API::clearRoute()
Optimize entity frame computation.
fix: Automated branch deletion is not working properly
…feature/unit_tests/simple_sensor_simulator
robomic
changed the base branch from
feature/simple_sensor_simulator_unit_test
to
feature/simple_sensor_simulator_unit_tests_part3
|
This PR contains newest changes from tier4/master and is built on this branch. |
There was a problem hiding this comment.
looks good to me!
We need to wait with this branch because it is based on a branch that hasn't been merged yet. To avoid submitting the same code for review twice, we should merge my branch first, and then yours. That's what I think would be the best approach.
| rclcpp::shutdown(); | ||
| server.join(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
you might consider adding a header guard also
Description
Abstract
This PR contains unit tests for multiple classes, all of them are a part of simple_sensor_simulator package.
Details
List of tests included in this PR:
References
Jira ticket: internal link
Destructive Changes
There are no destructive changes.