Move boxes_overlap tests from obb_test to boxes_overlap_test #21546
Conversation
|
Note to reviewers. The commits are (currently) curated. R1: This is a strict cut-and-paste. Code was lifted directly out of |
There was a problem hiding this comment.
+@rpoyner-tri for feature review, please.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
My first several attempts to use the visualization controls failed. I'm not a fan of fixed-delay-time interfaces, I guess.
May I suggest you use the scheme we set up for a bunch of Russ's tests? See drake/common/maybe_pause_for_user.h and the various tests that use it.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
My first several attempts to use the visualization controls failed. I'm not a fan of fixed-delay-time interfaces, I guess.
May I suggest you use the scheme we set up for a bunch of Russ's tests? See drake/common/maybe_pause_for_user.h and the various tests that use it.
None of that worked for me. Believe me, I tried.
The weird timing affordance is the only thing I could do to get the test to stay alive long enough to get a visualizer browser window populated.
Perhaps the simplest solution is to nuke the visualization.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
None of that worked for me. Believe me, I tried.
The weird timing affordance is the only thing I could do to get the test to stay alive long enough to get a visualizer browser window populated.
Perhaps the simplest solution is to nuke the visualization.
Nuke is fine with me; now I'm distracted by the fact that all of those other test visualizations appear to broken. But that's not for this PR to worry about.
SeanCurtis-TRI
force-pushed
the
PR_refactor_obb_overlap_tests
branch
from
21b970c to
7816034
Compare
There was a problem hiding this comment.
+@xuchenhan-tri for platform review, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Nuke is fine with me; now I'm distracted by the fact that all of those other test visualizations appear to broken. But that's not for this PR to worry about.
I have an alternative; I actually read the documentation for MaybePauseForUser() and updated the instructions. PTAL.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I have an alternative; I actually read the documentation for
MaybePauseForUser()and updated the instructions. PTAL.
BTW My note above about making sure we use the --visualize tag in CI may be incorrect. I don't think that's possible. C'est la vie.
There was a problem hiding this comment.
linters are whingeing; I could not quite tell why.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
xuchenhan-tri
added
the
release notes: none
There was a problem hiding this comment.
Platform
+(release notes: none)
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 2 of 2 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
minor, looks like the TODO in boxes_overlap.cc can now be retired.
geometry/proximity/test/boxes_overlap_test.cc line 145 at r5 (raw file):
} class BoxesOverlapTest_ : public ::testing::Test {
Why the trailing underscore?
geometry/proximity/test/boxes_overlap_test.cc line 198 at r5 (raw file):
// 9 cases use the axes defined by the cross product of axes from Frame A and // Frame B. We also test that it is robust for the case of parallel boxes. TEST_F(BoxesOverlapTest_, AllCases) {
BTW, the test name ("AllCases") seems to suggest that the set of one test fixture is closed. In that case you could just use SetUp instead of SetUpTestSuites and get rid of all the static keywords.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5.
Reviewable status: 3 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
The visualization code path should be somehow exercised in CI.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, xuchenhan-tri wrote…
The visualization code path should be somehow exercised in CI.
Suggestion:
I imagine it's easy to only guard the "pause for user" under the visualization gflag. Making the Meshcat and publishing during the test is still even even if nobody is watching.
Or really, I don't know why we have a gflag at all. The maybe-pause-for-user is inert during CI testing.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Suggestion:
I imagine it's easy to only guard the "pause for user" under the visualization gflag. Making the Meshcat and publishing during the test is still even even if nobody is watching.
Or really, I don't know why we have a gflag at all. The maybe-pause-for-user is inert during CI testing.
... is still fine even if ...
SeanCurtis-TRI
force-pushed
the
PR_refactor_obb_overlap_tests
branch
from
7816034 to
a13a00d
Compare
The code in boxes_overlap.cc was indirectly tested in obb_test.cc. Now boxes_overlap_test.cc takes the responsibility for testing the code directly (and obb_test.cc has become that much simpler). This also includes a tweak to MaybePauseForUser() to include an optional prompt message. The prompt message only gets printed if the pause is enabled.
SeanCurtis-TRI
force-pushed
the
PR_refactor_obb_overlap_tests
branch
from
a13a00d to
b4e2414
Compare
There was a problem hiding this comment.
Thanks.
One additional change for your eyes.
Reviewable status: 2 unresolved discussions
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... is still fine even if ...
I've removed the flag, shifted some documentation, and modified MaybePauseForUser to allow for user-defined prompt instructions that only get called if the pause is enabled.
geometry/proximity/test/boxes_overlap_test.cc line 145 at r5 (raw file):
Previously, xuchenhan-tri wrote…
Why the trailing underscore?
Done
It's simply one of life's great mysteries.
geometry/proximity/test/boxes_overlap_test.cc line 198 at r5 (raw file):
Previously, xuchenhan-tri wrote…
BTW, the test name ("AllCases") seems to suggest that the set of one test fixture is closed. In that case you could just use
SetUpinstead ofSetUpTestSuitesand get rid of all thestatickeywords.
I was planning on parameterizing them so that one could chose which case(s) they wanted to run. I think that's still a good idea, but I'm kicking it down the road. So, I'm inclined to leave it as is. However, I've added some documentation explaining the reasoning.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform)
The code in boxes_overlap.cc was indirectly tested in obb_test.cc. Now boxes_overlap_test.cc takes the responsibility for testing the code directly (and obb_test.cc has become that much simpler). This also includes a tweak to MaybePauseForUser() to include an optional prompt message. The prompt message only gets printed if the pause is enabled.
The code in boxes_overlap.cc was indirectly tested in obb_test.cc. Now boxes_overlap_test.cc takes the responsibility for testing the code directly (and obb_test.cc has become that much simpler). This also includes a tweak to MaybePauseForUser() to include an optional prompt message. The prompt message only gets printed if the pause is enabled.
This resolves a TODO in
boxes_overlap.cc. The function is no longer implicitly tested byobb_test.cc, but tested directly.Furthermore, the test is expanded to provide an affordance for visualizing the configurations (this helped in test authoring, clean up, and, I anticipate, future maintenance).
Relates to #21526
This change is