Skip to content

[JTC] Extend tests#612

Merged
bmagyar merged 7 commits intoros-controls:jtc-featuresfrom
christophfroehlich:jtc-extend-tests
Jun 20, 2023
Merged

[JTC] Extend tests#612
bmagyar merged 7 commits intoros-controls:jtc-featuresfrom
christophfroehlich:jtc-extend-tests

Conversation

@christophfroehlich
Copy link
Copy Markdown
Member

@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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
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
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.

2 participants