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

Refined RigidBodySystem unit test #2574

Merged
merged 3 commits into from
Jun 17, 2016

Conversation

liangfok
Copy link
Contributor

@liangfok liangfok commented Jun 17, 2016

Mostly adding more checks and fixing grammar and spelling issues in the comments.


This change is Reviewable

@liangfok
Copy link
Contributor Author

This PR passed https://drake-jenkins.csail.mit.edu/job/experimental/2116/ and is thus ready for review.
May I suggest:
+@sherm1 for feature review since he reviewed the previous version of this unit test.
+@ggould-tri for platform review since it's Friday.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@ggould-tri
Copy link
Contributor

Big readability improvements; thanks!

I have added one item to consider below. However because it is author's discretion I am applying platform :LGTM: anyway and you can decide if you want to bother.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake/systems/plants/test/rigid_body_system/rigid_body_system_test.cc, line 125 [r1] (raw file):

  EXPECT_NE(tree->findLink("link_3", "model_2", 1), nullptr);
  EXPECT_NE(tree->findLink("link_3", "model_1", 2), nullptr);
  EXPECT_NE(tree->findLink("link_3", "model_2", 3), nullptr);

Consider using a for loop here for conciseness (for (auto link : {"link1", ...}) { for (auto model : {"model_1", ...}) ...)

Author's discretion.


Comments from Reviewable

@liangfok
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake/systems/plants/test/rigid_body_system/rigid_body_system_test.cc, line 125 [r1] (raw file):

Previously, ggould-tri wrote…

Consider using a for loop here for conciseness (for (auto link : {"link1", ...}) { for (auto model : {"model_1", ...}) ...)

Author's discretion.

Unfortunately, using a `for` loop is not trivial because there are certain combinations that do not exist. For example, there's no such thing as <"link_1", "model_1", 1> or <"link_1", "model_2", 0>. Because of this, I'm going to merge as is.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 17, 2016

Feature review: :lgtm:; merging.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@sherm1 sherm1 merged commit efa2bc9 into RobotLocomotion:master Jun 17, 2016
@liangfok liangfok deleted the feature/multi_prius_demo branch June 17, 2016 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants