Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rclcpp/src/rclcpp/time.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ Time::operator-=(const rclcpp::Duration & rhs)
Time
Time::max()
{
return Time(std::numeric_limits<int32_t>::max(), 999999999);
return Time(std::numeric_limits<int64_t>::max());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Take a look to this change, it seemed to me that the previous value didn't have much sense as it is not near the max value described by header.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This came from the original commit back in 2018: #538 . While I definitely agree with you that this is weird, I think this deserves a separate PR. Particularly because rclcpp::Duration has the same problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@clalancette I didn't have context about that. Do you think I should open a PR to change that? The operators operating on Time/Duration seem to check for overflow/underflow, I don't see why the max value has to be limited.

If you think it requires further discussion I can open an issue instead of a PR, and for the time being adjust the time to pass with current implementation and reference the issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you think it requires further discussion I can open an issue instead of a PR, and for the time being adjust the time to pass with current implementation and reference the issue.

Yeah, I think it may need further discussion. So let's revert it here, make the test check for the current behavior (even though it is weird), and open a separate issue/PR to address fixing it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#1351 should follow up, and 7ba92dd changes the test to work with the implementation as is.

}


Expand Down
13 changes: 13 additions & 0 deletions rclcpp/test/rclcpp/test_duration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,16 @@ TEST_F(TestDuration, conversions) {
EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS);
}
}

TEST_F(TestDuration, test_some_constructors) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to add coverage for the exceptions thrown during the operators - (which calls bounds_check_duration_difference) and * (which just needs a check that multiplying by inf catches the appropriate exception)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll add them before merging! checking Windows CI first

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, see 049ba01

builtin_interfaces::msg::Duration duration_msg;
duration_msg.sec = 1;
duration_msg.nanosec = 1000;
rclcpp::Duration duration_from_msg(duration_msg);
EXPECT_EQ(RCL_S_TO_NS(1) + 1000, duration_from_msg.nanoseconds());

rcl_duration_t duration_struct;
duration_struct.nanoseconds = 4000;
rclcpp::Duration duration_from_struct(duration_struct);
EXPECT_EQ(4000, duration_from_struct.nanoseconds());
}
92 changes: 92 additions & 0 deletions rclcpp/test/rclcpp/test_time.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include "rclcpp/time.hpp"
#include "rclcpp/utilities.hpp"

#include "../utils/rclcpp_gtest_macros.hpp"

namespace
{

Expand Down Expand Up @@ -352,3 +354,93 @@ TEST_F(TestTime, seconds) {
EXPECT_DOUBLE_EQ(4.5, rclcpp::Time(4, 500000000).seconds());
EXPECT_DOUBLE_EQ(2.5, rclcpp::Time(0, 2500000000).seconds());
}

TEST_F(TestTime, test_max) {
rclcpp::Time time_max = rclcpp::Time::max();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of these rclcpp::Time variables you are setting up in your tests can be const. Fix here and below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, fixed here: 095bd0f

rclcpp::Time max_time(std::numeric_limits<int64_t>::max());
EXPECT_DOUBLE_EQ(max_time.seconds(), time_max.seconds());
EXPECT_EQ(max_time.nanoseconds(), time_max.nanoseconds());
}

TEST_F(TestTime, test_constructor_from_rcl_time_point) {
const rcl_time_point_value_t test_nano_seconds = 555;
const rcl_clock_type_t test_clock_type = RCL_ROS_TIME;
rcl_time_point_t test_time_point;
test_time_point.nanoseconds = test_nano_seconds;
test_time_point.clock_type = test_clock_type;

rclcpp::Time time_max = rclcpp::Time(test_time_point);

EXPECT_EQ(test_nano_seconds, time_max.nanoseconds());
EXPECT_EQ(test_nano_seconds, test_time_point.nanoseconds);
EXPECT_EQ(test_clock_type, time_max.get_clock_type());
EXPECT_EQ(test_clock_type, test_time_point.clock_type);
}

TEST_F(TestTime, test_assignment_operator_from_builtin_msg_time) {
rclcpp::Clock ros_clock(RCL_ROS_TIME);
builtin_interfaces::msg::Time ros_now = ros_clock.now();
EXPECT_NE(0, ros_now.sec);
EXPECT_NE(0u, ros_now.nanosec);

rclcpp::Time test_time(0u, RCL_CLOCK_UNINITIALIZED);
EXPECT_EQ(0u, test_time.nanoseconds());
EXPECT_EQ(RCL_CLOCK_UNINITIALIZED, test_time.get_clock_type());

test_time = ros_now;
EXPECT_NE(0, test_time.nanoseconds());
// The clock type is hardcoded internally
EXPECT_EQ(RCL_ROS_TIME, test_time.get_clock_type());
}

TEST_F(TestTime, test_sum_operator) {
rclcpp::Duration one(1);
rclcpp::Time test_time(0u);
EXPECT_EQ(0u, test_time.nanoseconds());

rclcpp::Time new_time = one + test_time;
EXPECT_EQ(1, new_time.nanoseconds());
}

TEST_F(TestTime, test_overflow_underflow_throws) {
rclcpp::Time test_time(0u);

RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Time(INT64_MAX) + rclcpp::Duration(1),
std::overflow_error("addition leads to int64_t overflow"));
RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Time(INT64_MIN) + rclcpp::Duration(-1),
std::underflow_error("addition leads to int64_t underflow"));

RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Time(INT64_MAX) - rclcpp::Duration(-1),
std::overflow_error("time subtraction leads to int64_t overflow"));
RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Time(INT64_MIN) - rclcpp::Duration(1),
std::underflow_error("time subtraction leads to int64_t underflow"));

test_time = rclcpp::Time(INT64_MAX);
RCLCPP_EXPECT_THROW_EQ(
test_time += rclcpp::Duration(1),
std::overflow_error("addition leads to int64_t overflow"));
test_time = rclcpp::Time(INT64_MIN);
RCLCPP_EXPECT_THROW_EQ(
test_time += rclcpp::Duration(-1),
std::underflow_error("addition leads to int64_t underflow"));

test_time = rclcpp::Time(INT64_MAX);
RCLCPP_EXPECT_THROW_EQ(
test_time -= rclcpp::Duration(-1),
std::overflow_error("time subtraction leads to int64_t overflow"));
test_time = rclcpp::Time(INT64_MIN);
RCLCPP_EXPECT_THROW_EQ(
test_time -= rclcpp::Duration(1),
std::underflow_error("time subtraction leads to int64_t underflow"));

RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Duration(INT64_MAX) + rclcpp::Time(1),
std::overflow_error("addition leads to int64_t overflow"));
RCLCPP_EXPECT_THROW_EQ(
test_time = rclcpp::Duration(INT64_MIN) + rclcpp::Time(-1),
std::underflow_error("addition leads to int64_t underflow"));
}
2 changes: 2 additions & 0 deletions rclcpp/test/rclcpp/test_time_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ TEST_F(TestTimeSource, ROS_time_valid_attach_detach) {

ts.attachNode(node);
EXPECT_FALSE(ros_clock->ros_time_is_active());

ts.detachClock(ros_clock);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there anything you can check to make sure detachClock worked as expected?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a check similar to the rest of the test scenario here: 5d54798

I didn't see any straightforward way to test it out, the enable_ros_time and disable_ros_time could be great for that, but those are private. Let me know if you have any clue how to test this out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't see any straightforward way to test it out, the enable_ros_time and disable_ros_time could be great for that, but those are private. Let me know if you have any clue how to test this out.

We could consider making those protected and deriving the test class from it, but it's not required.

}

TEST_F(TestTimeSource, ROS_time_valid_wall_time) {
Expand Down