Skip to content
This repository has been archived by the owner on Jul 22, 2021. It is now read-only.

Unit tests for Trajectory API #2

Merged
merged 18 commits into from
Sep 30, 2019

Conversation

cnboonhan
Copy link
Contributor

This PR is to introduce unit tests for the Trajectory API in rmf_core. I have flagged possible failing tests using the keyword FLAG and commented them out.

Where possible I have tried to check move and copy constructors and their side effects. I may not have done them 100% correctly, would be good to double check how valid the tests are.

The tests are written BDD style: the logical meta-structure is as follows:

  • For each class, starting from the most atomic,

  • Test creation of the class,

  • Test the class functions

Copy link
Member

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Here's a preliminary review of what I've looked through so far. I'm only about halfway through test_trajectory.cpp at the moment.

These tests are really fantastic and comprehensive, so thanks for putting all this time and thought into them. You'll find some specific remarks about specific lines below, but I'll also add one general remark:

I strong recommend making liberal use of the const qualifier in your variable declarations. You should always default to declaring variables as const unless you definitely need to modify their values later. That's not anything specific to writing unit tests, but more of a general C++ programming recommendation.

rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
Copy link
Member

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Most of feedback is really minor, just small tweaks to improve the code quality.

Thanks again for writing all these tests!

rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Outdated Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Show resolved Hide resolved
rmf_traffic/test/unit/test_trajectory.cpp Show resolved Hide resolved
@cnboonhan
Copy link
Contributor Author

will add const declarations, and follow the pointer conventions, in a final pass before merging.

@cnboonhan
Copy link
Contributor Author

cnboonhan commented Sep 30, 2019

I have added many consts ( the best consts ). Sorry if I have missed out anything: i am assuming that as everything is marked as resolved, that I do not need to track them.

If everything is ok, I would like to merge this as well as delete the refactor traffic_controller_add_tests branchwhich is now redundant.

@mxgrey
Copy link
Member

mxgrey commented Sep 30, 2019

Those are some very strong consts, some would even say the strongest.

Great job with these tests; I'm happy to merge it now.

@mxgrey mxgrey merged commit 53454d5 into rmf_traffic_controller-prototype Sep 30, 2019
@mxgrey mxgrey deleted the traffic_controller_add_tests branch September 30, 2019 11:09
ddengster added a commit that referenced this pull request Aug 5, 2020
ddengster added a commit that referenced this pull request Aug 5, 2020
ddengster added a commit that referenced this pull request Aug 5, 2020
ddengster added a commit that referenced this pull request Aug 14, 2020
ddengster added a commit that referenced this pull request Aug 14, 2020
ddengster added a commit that referenced this pull request Aug 14, 2020
ddengster added a commit that referenced this pull request Dec 8, 2020
Co-authored-by: Grey <grey@openrobotics.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants