Clean up pass on geometry state tests

[SeanCurtis-TRI]

This addresses a number of style and cosmetic issues encountered while
integrating rendering into geometry state. This isolates the legacy issues
so that new code can be considered without this noise in the signal.

This doesn't change the functionality of the tests. It only changes
spellings and the like:

1. Use of aliases to make code more compact.
2. Added const to tons of local variables.
3. Refactored the GeometryStateTest core into a new calss:
   GeometryStateTestBase (this will facilitate new tests in upcoming PRs).
   • Change the interface to SetUpSingleSourceTree() to make call sites
     more readable.
   • Remove to methods that are no longer used:
     • ExpectSourceDoesNotHaveFrames()
     • AssertSingleTreeCleared()
   • Rename vectors of poses: e.g., X_WF_ --> X_WFs_.
4. Make more fluent use of pose data (will lend itself to translation to
   RigidTransform).
5. Clarify, clean up comments.
6. Move ProximityRoleOnMesh to bottom.

---

This change is 

[SeanCurtis-TRI]

+@DamrongGuoy for feature review.

NOTE: This is just a clean up/refactor. There is no new functionality.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

[DamrongGuoy]

with minor requests.

Reviewed 1 of 1 files at r1.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

---

geometry/test/geometry_state_test.cc, line 1151 at r1 (raw file):

  // Add an anchored geometry.
  const SourceId s_id = NewSource("new source");
  Isometry3d pose{Translation3d{1, 2, 3}};

Nit, const Isometry3d pose ?

---

geometry/test/geometry_state_test.cc, line 2001 at r1 (raw file):

  // are at (5, 0, 0) and (6, 0, 0), respectively. Split the distance to put the
  // new geometry in a penetrating configuration.
  Isometry3d pose{Translation3d{5.5, 0, 0}};

Nit, const Isometry3d pose ?

[SeanCurtis-TRI]

Thanks @DamrongGuoy, +@soonho-tri for platform review.

Reviewable status: LGTM missing from assignee soonho-tri(platform) (waiting on @soonho-tri)

---

geometry/test/geometry_state_test.cc, line 1151 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit, const Isometry3d pose ?

OK Not in this case, it's set five lines lower down.

---

geometry/test/geometry_state_test.cc, line 2001 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit, const Isometry3d pose ?

Done

[SeanCurtis-TRI]

-@soonho-tri +@sammy-tri for platform review; apparently still enough time to sneak it in today.

Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @DamrongGuoy and @sammy-tri)

[sammy-tri]

Reviewed 1 of 1 files at r2.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @SeanCurtis-TRI)

---

geometry/test/geometry_state_test.cc, line 311 at r2 (raw file):

// create arbitrary test harnesses using the same infrastructure. (See
// GeometryStateTest and RemoveRoleTests -- this latter comes in a future PR.)
class GeometryStateTestBase {

Do we expect that this will be used in derived classes which don't inherit from ::testing::Test? I'm wondering if we should inherit directly (or use composition) here instead of making a class where the correct way to use is always involves multiple inheritance. (from https://drake.mit.edu/styleguide/cppguide.html#Inheritance "multiple implementation inheritance is strongly discouraged")

If you've got a pointer to a branch with the code for an upcoming PR as mentioned above, I could look there and see if it makes more sense to me.

---

geometry/test/geometry_state_test.cc, line 313 at r2 (raw file):

class GeometryStateTestBase {
 public:
  GeometryStateTestBase() = default;

Related to above, the public constructor is mildly confusing on the question of if other tests should inherit from this class or use composition (maybe either is really OK and I'm being too picky about test code?)

---

geometry/test/geometry_state_test.cc, line 315 at r2 (raw file):

  GeometryStateTestBase() = default;

  // The initialization to be done prior to each test; the base test class

minor: does "base test class" here mean "derived test class"?

[SeanCurtis-TRI]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)

---

geometry/test/geometry_state_test.cc, line 311 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Do we expect that this will be used in derived classes which don't inherit from ::testing::Test? I'm wondering if we should inherit directly (or use composition) here instead of making a class where the correct way to use is always involves multiple inheritance. (from https://drake.mit.edu/styleguide/cppguide.html#Inheritance "multiple implementation inheritance is strongly discouraged")

If you've got a pointer to a branch with the code for an upcoming PR as mentioned above, I could look there and see if it makes more sense to me.

I derive two tests from it. One a ::testing::Test and once as a ::testing::TestWithParam. That makes this a mix-in. I briefly toyed with making this a standalone struct that both of those would instantiate, but then my code blew up with the additional indirection.

If you feel the multiple-inheritance is too evil, even for a unit test, I'll push this in the other direction where it's a "has-a" with more characters.

---

geometry/test/geometry_state_test.cc, line 313 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Related to above, the public constructor is mildly confusing on the question of if other tests should inherit from this class or use composition (maybe either is really OK and I'm being too picky about test code?)

I'll defer a statement here until you see my previous comment.

---

geometry/test/geometry_state_test.cc, line 315 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

minor: does "base test class" here mean "derived test class"?

I'll address this based on the bigger topic (above).

[DamrongGuoy]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)

---

geometry/test/geometry_state_test.cc, line 1151 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK Not in this case, it's set five lines lower down.

OK. Thanks.

[DamrongGuoy]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)

[sammy-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @SeanCurtis-TRI)

---

geometry/test/geometry_state_test.cc, line 311 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I derive two tests from it. One a ::testing::Test and once as a ::testing::TestWithParam. That makes this a mix-in. I briefly toyed with making this a standalone struct that both of those would instantiate, but then my code blew up with the additional indirection.

If you feel the multiple-inheritance is too evil, even for a unit test, I'll push this in the other direction where it's a "has-a" with more characters.

Ok, I'll buy that.

---

geometry/test/geometry_state_test.cc, line 313 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'll defer a statement here until you see my previous comment.

I guess it could be useful on it's own? Given that it's local to a unit test the public/protected distinction probably isn't that meaningful.

---

geometry/test/geometry_state_test.cc, line 315 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'll address this based on the bigger topic (above).

I think we're agreed on not changing the design, so I'm still leaning toward "derived".

[SeanCurtis-TRI]

Reviewable status: LGTM missing from assignee sammy-tri(platform)

---

geometry/test/geometry_state_test.cc, line 315 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I think we're agreed on not changing the design, so I'm still leaning toward "derived".

Done

[DamrongGuoy]

Reviewed 1 of 1 files at r3.
Reviewable status: LGTM missing from assignee sammy-tri(platform)

[sammy-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),DamrongGuoy