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

[JTC] Extend tests #612

Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented May 14, 2023

In preparation for writing new tests for the upcoming changes:

  • I reactivated commented tests in test_trajectory_controller
  • Added parameterized tests for test_trajectory_actions.

If tests were deactivated for other reasons than being flaky, please let me know ;)

Three tests are failing, and I don't know if the test logic or JTC behavior is broken. I added comments below

// reactivated
// wait so controller process the third point when reactivated
std::this_thread::sleep_for(std::chrono::milliseconds(3000));
// TODO(anyone) test copied from ROS 1: it fails now!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged the old reactivation test into that one, should the old trajectory really be processed after reactivation? (it doesn't)

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't imo and we should have a test to ensure it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree. Would be strange if it does. Writing the test for that is easy, because this one fails already :D

expected_actual, expected_desired, executor, rclcpp::Duration(delay * (2 + 2)), 0.1);
}

// TODO(destogl) this test fails with errors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@destogl maybe you can have a quick look on that?

}
#endif

// TODO(destogl) this test fails
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@destogl maybe you can have a quick look on that?

// if using RCL_STEADY_TIME ->
// C++ exception with description
// "can't compare times with different time sources" thrown in the test body.
// traj_controller_->update(clock.now(), clock.now() - start_time);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we create the node_ with NodeOptions, clock_type = RCL_STEADY_TIME?

Copy link
Member

Choose a reason for hiding this comment

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

Some of these may be from a time we didn't have any clock solution, worth reassessing if needed

@bmagyar
Copy link
Member

bmagyar commented May 14, 2023

Confirming here that the only reason I remember tests were disabled was flakiness

@bmagyar bmagyar merged commit 42b2590 into ros-controls:jtc-features Jun 20, 2023
7 checks passed
bmagyar pushed a commit that referenced this pull request Jun 26, 2023
christophfroehlich added a commit to christophfroehlich/ros2_controllers that referenced this pull request Jul 17, 2023
bmagyar pushed a commit that referenced this pull request Jul 17, 2023
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
(cherry picked from commit 9908a9c)

# Conflicts:
#	joint_trajectory_controller/test/test_trajectory_controller.cpp
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
(cherry picked from commit 9908a9c)

# Conflicts:
#	joint_trajectory_controller/test/test_trajectory_controller.cpp
@christophfroehlich christophfroehlich deleted the jtc-extend-tests branch July 25, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants