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.

Sign up for GitHub

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

Jump to bottom

More jtc tests #75

Merged
merged 11 commits into from
Sep 9, 2020
Merged

More jtc tests #75

merged 11 commits into from
Sep 9, 2020

Conversation

v-lopez
Copy link
Contributor

@v-lopez v-lopez commented Jul 1, 2020

These changes originate from the partial-joint branch in #68

Let's wait until that is merged before accepting this one.

@v-lopez v-lopez mentioned this pull request Jul 1, 2020
Open
@v-lopez v-lopez force-pushed the more-jtc-tests branch 5 times, most recently from dab98a7 to a923054 Compare July 7, 2020 10:12
@bmagyar bmagyar self-requested a review July 18, 2020 09:44
@bmagyar bmagyar marked this pull request as ready for review July 18, 2020 09:45
@bmagyar
Copy link
Member

bmagyar commented Jul 19, 2020

I've squashed & rebased this to the latest master, with the recent merges to master the merge algorithm was confused, there were no conflicts really.

@bmagyar
Copy link
Member

bmagyar commented Jul 19, 2020

@v-lopez I've created a backup of the old branch at pal/more-jtc-tests-orig

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2020 •
edited
Loading

Codecov Report

Merging #75 into master will increase coverage by 3.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   33.12%   36.19%   +3.06%     
==========================================
  Files          12       23      +11     
  Lines        1123     2310    +1187     
  Branches      740     1424     +684     
==========================================
+ Hits          372      836     +464     
- Misses        103      200      +97     
- Partials      648     1274     +626
Flag Coverage Δ
#unittests 36.19% <ø> (+3.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...include/joint_trajectory_controller/trajectory.hpp 46.66% <0.00%> (-20.00%) ⬇️
...int_trajectory_controller/test/test_trajectory.cpp 17.72% <0.00%> (-8.66%) ⬇️
.../joint_state_controller/joint_state_controller.hpp 100.00% <0.00%> (ø)
...jectory_controller/joint_trajectory_controller.hpp 100.00% <0.00%> (ø)
...te_controller/test/test_joint_state_controller.hpp 100.00% <0.00%> (ø)
...diff_drive_controller/rolling_mean_accumulator.hpp 85.71% <0.00%> (ø)
...ollers/diff_drive_controller/src/speed_limiter.cpp 54.54% <0.00%> (ø)
...te_controller/test/test_joint_state_controller.cpp 19.65% <0.00%> (ø)
...controllers/diff_drive_controller/src/odometry.cpp 68.42% <0.00%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 27.27% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8587e07...9678dd2. Read the comment docs.

bmagyar
bmagyar requested changes Jul 19, 2020
View reviewed changes
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thanks for porting these over! I have a few minor points.
Is this all the remaining ROS1 jtc tests or are there more?

joint_trajectory_controller/src/joint_trajectory_controller.cpp Show resolved Hide resolved
joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated Show resolved Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp Show resolved Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp Outdated Show resolved Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp Outdated Show resolved Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp Outdated Show resolved Hide resolved
joint_trajectory_controller/test/test_trajectory_controller.cpp Show resolved Hide resolved
@v-lopez
Copy link
Contributor Author

v-lopez commented Jul 19, 2020

Thanks for the review.

I cannot address thisuuntil the week after the next one.

In #49 I am writing down the ROS1 tests in each file, I believe these cover just 3 files, there's a bunch more. I tried to keep below the 500 line limit suggestion

@v-lopez v-lopez requested a review from bmagyar July 28, 2020 11:02
@v-lopez v-lopez force-pushed the more-jtc-tests branch 2 times, most recently from 972b1c8 to 69bc409 Compare July 28, 2020 12:57
@v-lopez
Copy link
Contributor Author

v-lopez commented Jul 30, 2020

Should I keep adding tests to this PR or continue on a separate branch?

@bmagyar
Copy link
Member

bmagyar commented Aug 3, 2020

@v-lopez I think this is a good amount of new stuff to be merged, please open a new PR with additional tests.

@v-lopez
Copy link
Contributor Author

v-lopez commented Aug 3, 2020

Will do!

Victor Lopez and others added 4 commits August 18, 2020 08:48
Extend trajectory tests to check velocities and accelerations
25d17ec 
Remove duplicated code
Make JTC members protected for easier extension and tests
Add invalid_message test
Remove sleeps and cleanup tests
Fix test errors detected with valgrind
Cleanup tests, extend invalid_names and add replace trajectory test
Reject old trajectories
Add ignore old trajectories test
@v-lopez @bmagyar
Update joint_trajectory_controller/src/joint_trajectory_controller.cpp
5454199 
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Apply PR suggestions
db8961f
Add test with not allowed partial joints
8049e06
@v-lopez
Copy link
Contributor Author

v-lopez commented Aug 18, 2020

@bmagyar I've rebased this and linters and tests are passing on my machine. Should be good to go when the actions finish.

@v-lopez v-lopez mentioned this pull request Aug 19, 2020
Open
bmagyar
bmagyar approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thanks very much for this, this is a huge improvement!

@bmagyar bmagyar merged commit d529025 into ros-controls:master Sep 9, 2020
gwalck pushed a commit to StoglRobotics-forks/ros2_controllers that referenced this pull request Jun 7, 2023
@matthew-reynolds
Remove test_depend from metapackage (ros-controls#75)
cb4d4b4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@v-lopez @bmagyar @codecov-commenter