Test installed drake bazel macro

[fbudin69500]

Now that the resources have been moved one directory up in drake's install
folder, we need to also move the resource sentinel.

---

This change is 

[fbudin69500]

@m-chaturvedi There was a bug in the resource finding process. This PR should solve your Shambala issue.

[jwnimmer-tri]

I was hoping to wait to alter the install tree's layout until we've settled the questions like #7586 about whether we are going to put back the "drake" folder -- I would prefer not to thrash our users if we're just going to undo it again in a few weeks.

I don't think the current install is defective? The intent was that it operates the same as it used to before the rename. If there is some problem, please write up the bug report (inline in this PR discussion is fine) so that I can understand the problem. I didn't immediately see any open issue on Shambhala that related to this.

---

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

---

Comments from Reviewable

[fbudin69500]

With master, if one tries to run an example, such as kuka_plan_runner, and the current working directory is outside of the build directory (otherwise the issue is hidden as .drake-find_resource-sentinel is found in the build directory), they will get the following error message:

terminate called after throwing an instance of 'std::runtime_error'
  what():  could not find resource: drake/manipulation/models/iiwa_description/urdf/iiwa14_primitive_collision.urdf
[1]    24253 abort (core dumped)  /tmp/drake/share/drake/examples/kuka_iiwa_arm/kuka_plan_runner```

---

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

---

Comments from Reviewable

[jwnimmer-tri]

Oh, I see what happened.

In ./examples/kuka_iiwa_arm/models (and elsewhere) we use install_data, which in its implementation does data_dest = "share/drake/" + native.package_name(). Because the package_name changed, most of the installed files ended up in a different place. That was not my intent.

I can see now how this PR fixes the (remaining) paths to match with the de-duplicated drake. The inconsistency on master right now is definitely a defect.

My question is whether we either (1) go the route of this PR by updating the non-install_data rules so that we don't have the double-drake anymore, or whether we (2) for now just fix the places that use native.package_name() for data installs (the macro, and at lease one other BUILD file that hard-codes that expression) to add the "drake" back in so that we retain the prior layout.

I will give it some further thought.

---

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

---

Comments from Reviewable

[fbudin69500]

Ok. Personally I have no preference over one or the other solution, although the double drake folder does seem redundant.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

OK So I think this PR is in the right direction; we'll drop the duplication and square everything up to that new convention.

At a minimum, this PR needs a regression test that fails without the fixes and passes with them -- running the kuka thing from the install directory, or something? Or a simpler (new?) example program with the same failure mode would be OK also.

Longer term, we are gaining more and more "test the install" tests which is great, but I'm wondering now if we should be collecting or consolidating them a bit for clarity / conciseness / efficiency, such as all declared in the same subdirectory -- at least, all of them that depend on the monolithic //:install target. We should also give some thought to narrowing the hazard gap between "everything works from the simple bazel build" vs "I installed it and now its broken" -- ideally there is no room for error there, so that we don't need to many tests. But I'm starting to lean toward now that everything that is installed needs a regression test that proves once its been installed, it still works. That's all for a separate PR, though.

For this PR review, I also need to take a fine comb and think about any other modalities or documentation we might have missed in terms of de-duplicating the data install directory. I will work on that.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Would it be hard to remove the appending of drake/ when adding a resource path?
(Following up from #7809)

[jwnimmer-tri]

@EricCousineau-TRI I'm not sure what you're asking specifically? This PR changes the install tree (1) to be internally consistent with the various items installed and (2) to have only one drake in the absolute pathnames, not drake/drake. Is that what you mean?

[jwnimmer-tri]

LGTM pending the open discussions.

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

a discussion (no related file):
The tools/install/install_data.bzl comments around L24 also need fixing.

---

a discussion (no related file):
This PR needs a regression test that fails without the fixes and passes with them -- running the kuka_simulation from the install directory for example, or something similar? Or a simpler (new?) example program with the same failure mode would be OK also. Or

---

common/BUILD.bazel, line 396 at r1 (raw file):

    # package name in the installed path.  For now, we'll just specify it
    # manually.
    runtime_dest = "libexec/drake/drake/common",

For consistency, this should have one less drake.

(Possibly it could move to share/drake/common/resource_tool if you want, since I guess we're okay installing platform-dependent binaries there already (the examples).)

---

common/find_resource.cc, line 239 at r1 (raw file):

  }
  const std::string resource_path_substr = resource_path.substr(prefix.size());

This code-paragraph break got lost.

---

Comments from Reviewable

[EricCousineau-TRI]

Posted my qualms as a BTW; namely was frustrated with how complicated / opaque this mechanism is.
Would be nicer if paths were exactly what were searched, and inputs were not adjusted magically (even if that means the user has to adjust their environment variable once or twice).

---

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

common/find_resource.cc, line 247 at r1 (raw file):

  // (1) Search the environment variable first; if it works, it should always
  // win.  TODO(jwnimmer-tri) Should we split on colons, making this a PATH?
  candidate_dirs.emplace_back(AppendDrakeTo(getenv_optional(

FYI The usage of AppendDrakeTo is part of what I really don't like for debugging: more or less opaquely appending /drake to environment variables and user-provided search paths. It's confusing, even it's documented (which it doesn't appear to be).
When I was debugging, it was really unclear what was actually being sought until I dug into the code to untangle all of the logic.

Is there any reason that this cannot simpler (less Drake-specific), so that the logic is much simpler?
Steps I can think of would be to (a) remove any usage of AppendDrakeTo and (b) ensure that every possibly search directory is exposed to the user for debugging in Python (so that getting an empty list in pydrake.common.GetResourceSearchPaths truly means that nothing will be found)?

I understand that these changes are probably out-of-scope for this PR, so I would be happy to make this a separate issue, and change this myself (if there are no strong reasons for having this design currently as-is).

---

Comments from Reviewable

[EricCousineau-TRI]

To add another minor comment, it does seem like FindResource really shouldn't be searching a list of candidate directories every time.

On initialization, it should check for "the one true directory", and only use that directory.
Anything more complicated than that (searching along multiple distinct trees) is something that should be done by another tool (e.g. ROS package paths).

Given that my comments don't really contribute much to this PR, I will create a separate issue - sorry for the noise!

[fbudin69500]

Review status: 0 of 11 files reviewed at latest revision, 5 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR needs a regression test that fails without the fixes and passes with them -- running the kuka_simulation from the install directory for example, or something similar? Or a simpler (new?) example program with the same failure mode would be OK also. Or

A test that fails is difficult (and that is why this bug was introduced without any test failing) because of the sentinel that is in the root directory. If the detection of the sentinel fails in the install directory, it will detect the sentinel in the source tree, and find the data from there.

---

common/BUILD.bazel, line 396 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For consistency, this should have one less drake.

(Possibly it could move to share/drake/common/resource_tool if you want, since I guess we're okay installing platform-dependent binaries there already (the examples).)

Done.

---

common/find_resource.cc, line 239 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This code-paragraph break got lost.

Done.

---

common/find_resource.cc, line 247 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

FYI The usage of AppendDrakeTo is part of what I really don't like for debugging: more or less opaquely appending /drake to environment variables and user-provided search paths. It's confusing, even it's documented (which it doesn't appear to be).
When I was debugging, it was really unclear what was actually being sought until I dug into the code to untangle all of the logic.

Is there any reason that this cannot simpler (less Drake-specific), so that the logic is much simpler?
Steps I can think of would be to (a) remove any usage of AppendDrakeTo and (b) ensure that every possibly search directory is exposed to the user for debugging in Python (so that getting an empty list in pydrake.common.GetResourceSearchPaths truly means that nothing will be found)?

I understand that these changes are probably out-of-scope for this PR, so I would be happy to make this a separate issue, and change this myself (if there are no strong reasons for having this design currently as-is).

I overall agree with this statement. I added a comment in issue #7821 , but this should be treated in a different PR.

---

Comments from Reviewable

[fbudin69500]

I added a new macro to easily test executables after installation. It only requires to install drake once. It can probably cleaned up or improved a bit, but I thought I would give it a first shot in this PR.

---

Review status: 0 of 11 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[fbudin69500]

Review status: 0 of 11 files reviewed at latest revision, 4 unresolved discussions.

---

common/BUILD.bazel, line 396 at r1 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

Done.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint. I will look at the new test-provider infrastructure a bit later on.

---

Reviewed 3 of 5 files at r3, 2 of 2 files at r4, 1 of 6 files at r5.
Review status: 6 of 11 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, fbudin69500 (Francois Budin) wrote…

A test that fails is difficult (and that is why this bug was introduced without any test failing) because of the sentinel that is in the root directory. If the detection of the sentinel fails in the install directory, it will detect the sentinel in the source tree, and find the data from there.

Isn't that why we remove the non-installed copies (

drake/tools/install/install_test_helper.py

Line 29 in 122e44b

# Remove Bazel build artifacts, and ensure that we only have install

) to get a clean answer? (Setting cwd=/ seems like a cute hack for the same effect.) Now that this PR uses that helper for, e.g., resource_tool_installed_test, is it good enough?

Aside: In any case, it turns out that writing test data into . from a unit test is a defect per https://docs.bazel.build/versions/master/test-encyclopedia.html. We should have been using $TEST_TMPDIR instead. I believe that in the course of fixing that up (and assuming we cwd= into $TEST_TMPDIR when running the subprocess under test), we will be testing from a place that won't see the runfiles, so won't be confused. (Not to say that this PR needs to obey $TEST_TMPDIR since nothing else does yet, but FYI in case it helps.)

---

bindings/pydrake/test/common_install_test.py, line 36 at r5 (raw file):

             import pydrake; print(pydrake.getDrakePath())"
             ],
            cwd='/',

Comment that this is to avoid finding files through the "look in pardir" heuristic, since we want to exercise the libdrake.so heuristic? Is there some reason why intsall_test_helper's removal of the runfiles tree wasn't good enough?

---

tools/install/install.bzl, line 280 at r5 (raw file):

#------------------------------------------------------------------------------
# Compute install test actions

BTW punc

---

tools/skylark/drake_install_and_test.bzl, line 22 at r5 (raw file):

        data = ["//:install"],
        main = name + "TestCommands.py",
        tags = ["no_everything"],

Unclear why this is here (probably just needs a comment).

---

Comments from Reviewable

[fbudin69500]

Review status: 6 of 11 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Isn't that why we remove the non-installed copies (

drake/tools/install/install_test_helper.py

Line 29 in 122e44b

# Remove Bazel build artifacts, and ensure that we only have install

) to get a clean answer? (Setting cwd=/ seems like a cute hack for the same effect.) Now that this PR uses that helper for, e.g., resource_tool_installed_test, is it good enough?

Aside: In any case, it turns out that writing test data into . from a unit test is a defect per https://docs.bazel.build/versions/master/test-encyclopedia.html. We should have been using $TEST_TMPDIR instead. I believe that in the course of fixing that up (and assuming we cwd= into $TEST_TMPDIR when running the subprocess under test), we will be testing from a place that won't see the runfiles, so won't be confused. (Not to say that this PR needs to obey $TEST_TMPDIR since nothing else does yet, but FYI in case it helps.)

I wrote that previous comment before thinking about changing the current working directory, so my previous comment is now irrelevant.
Removing the bazel artifact partially solves the problem of finding the resources outside of the install directory, but not fully since if it doesn't find the resources, it will go up a few directories in the file system and eventually find the resources that are in the source folder. That is why I thought about changing the current working directory to something that is for sure outside of the source tree (i.e. '/').
Another PR could improve the current tests and use $TEST_TMPDIR, instead of '/' as the test current working directory, but we would have to make sure that $TEST_TMPDIR is not a subfolder of the source code folder, which I would anticipate it will be.

---

Comments from Reviewable

[jwnimmer-tri]

a discussion (no related file):

Previously, fbudin69500 (Francois Budin) wrote…

I wrote that previous comment before thinking about changing the current working directory, so my previous comment is now irrelevant.
Removing the bazel artifact partially solves the problem of finding the resources outside of the install directory, but not fully since if it doesn't find the resources, it will go up a few directories in the file system and eventually find the resources that are in the source folder. That is why I thought about changing the current working directory to something that is for sure outside of the source tree (i.e. '/').
Another PR could improve the current tests and use $TEST_TMPDIR, instead of '/' as the test current working directory, but we would have to make sure that $TEST_TMPDIR is not a subfolder of the source code folder, which I would anticipate it will be.

That's weird, because for me the current directory when tests are run (the runfiles) live under $HOME/.cache, not $HOME/foo/drake.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: 6 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

bindings/pydrake/test/common_install_test.py, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Comment that this is to avoid finding files through the "look in pardir" heuristic, since we want to exercise the libdrake.so heuristic? Is there some reason why intsall_test_helper's removal of the runfiles tree wasn't good enough?

Or maybe -- comment here as to why, and then since everything uses cwd=/ now, we can nix the "rmtree" logic?

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 6 files at r5.
Review status: 8 of 11 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

examples/BUILD.bazel, line 72 at r5 (raw file):

])

drake_install_and_test(

This infrastructure uses Bazel "providers" to locate the requested tests. This is cute because then far-flung install rules can consolidate their tests into a single runtime case, to avoid repeatedly running the install (which is I/O intensive and thus slow on CI).

The downside is that a developer might write a "test_commands" stanza that gets completely ignored, if no depending-on label like this one happens to say drake_install_and_test vs just install.

Proposal : To remove the possibility for user error, it seems like drake_install_and_test should be used only and exactly once, as applied to the //:install rule. (Whether the test case is declared in the root package (//:*), or in the //tools/install/... tree somewhere, I don't care. I could see advocating either way.)

---

examples/kuka_iiwa_arm/BUILD.bazel, line 20 at r5 (raw file):

    "//tools/skylark:drake_py.bzl",
    "drake_py_test",
)

BTW Unused?

---

examples/kuka_iiwa_arm/BUILD.bazel, line 227 at r5 (raw file):

        "iiwa_wsg_simulation": ["kill"],
        "kuka_plan_runner": ["kill"],
        "kuka_simulation": ["kill"],

So the brevity is cute, but I feel like we are quickly going to exceed what this is capable of (for example, passing command-line arguments). The details of test cases are usually spelled out in test code, not BUILD rules; the BUILD rules usually just enumerate them.

Proposal: Is there some way here we could pass the label for a test script file, instead of just a keyword? The file could still have some way to reuse the "run" and "runAndKill" primitives (maybe through an import), but then we can use the full power of Python to phrase the tests, instead of threading it through providers and codegen.

---

tools/install/install_tests.py.in, line 1 at r5 (raw file):

#!/usr/bin/env python

The *.py.in pattern has poor maintainability as per #7331; let's not do that again.

The easy change is that the <<actions>> text is emitted by the bzl into a data file, which this code then opens and read lines from.

(Depending on how the "test_commands" discussion above gets solved (citing test code, instead of keyword primitives, to enumerate test cases), this runner might change substantially anyway, I guess.)

---

tools/skylark/drake_install_and_test.bzl, line 3 at r5 (raw file):

# -*- python -*-

load("//tools/install:install_data.bzl", "install")

I think it might make more sense for this .bzl file to live adjacent to the code its tightly coupled with (//tools/install:install.bzl)? We directly depend on several of the labels and skylark infrastructure in there.

(FYI We could also drop drake from this name, since its using the test_commands details that are present in the install.bzl rules directly, not some Drake-specific subclassing of install rules.)

---

tools/skylark/drake_install_and_test.bzl, line 9 at r5 (raw file):

        name,
        **kwargs):
    """A wrapper to install and test drake executables."""

This doc will need to be expanded so that readers can understand what it's doing. Calling out test_commands from the install rule would be one important piece.

---

tools/skylark/drake_install_and_test.bzl, line 12 at r5 (raw file):

    install(
        name = name,
        **kwargs)

One macro that creates multiple rules is in general confusing to readers (names appear out of nowhere). Would you consider removing the install part here, so this macro just becomes install_test or similar?

---

Comments from Reviewable

[jamiesnape]

Reviewed 1 of 6 files at r5, 1 of 15 files at r6, 18 of 19 files at r7, 10 of 10 files at r8, 6 of 7 files at r9, 13 of 13 files at r10.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jwnimmer-tri]

FYI For some of the merge conflicts, if you've rewritten test cases using the install_tests idiom, and I had added a TODO() to port them to drake_py_unittest, then a fine resolution is to remove the TODO; the point there was to stop using drake_py_test which this PR does.

[fbudin69500]

I updated to py and remove the fact that it is configured. It now loads a file that is passed as an argument. This PR does not take full advantage of drake_py_unittest as this macro calls a script that looks for the test script in a specific location that may not play well with where the install test file is. Another PR could be done after to improve this.

---

Review status: 9 of 20 files reviewed at latest revision, 7 unresolved discussions.

---

common/test/resource_tool_installed_test.py, line 19 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I meant basically this:

mkdir -p TMPDIR/tmp/share/drake/common/test
ln -s \
  TMPDIR/install/share/drake/.drake-find_resource_sentinel
  TMPDIR/tmp/share/drake/.drake-find_resource_sentinel

... replacing open + write with just symlink.

Done.

---

common/test/resource_tool_installed_test.py, line 22 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The assumption here is that the sentinel is named .drake-find_resource-sentinel as in https://github.com/RobotLocomotion/drake/blob/master/common/find_resource.cc#L160. This test program should fail-fast that assumption changes, which it can check for example by seeing that the original dotfile in the install tree exists under that name.

Done.

---

tools/install/install.bzl, line 93 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The target argument does not appear to be used?

Done.

---

tools/install/install.bzl, line 401 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Please a fail-fast if the emptiness of install_tests_script is inconsistent with the emptiness of script_tests. To my reading, they should both be empty or both be non-empty.

install_tests_script can be empty while script_tests is not if one wants to create some install tests (and set providers), but doesn't want the tests to be run from here. The other way around now returns an error.

---

tools/install/install.bzl, line 542 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The above paragraph is stale.

Done.

---

tools/install/install.bzl, line 765 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI We could enforced with https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name here, if we wanted to.

Done.

---

tools/install/test/install_tests.py, line 26 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm unclear on why this check happens after the cmds; does it matter? If so, please clarify with a comment.

hum, I don't know why it ended up there. It was not my plan. I moved it before the cmds.

---

Comments from Reviewable

[jwnimmer-tri]

Final replies posted. I think after those are resolved and CI is happy, this is ready for a fresh set of reviewer eyes. I think tagging Soonho for Thursday's platform review of this PR would be good.

This PR does not take full advantage of drake_py_unittest as this macro calls a script that looks for the test script in a specific location that may not play well with where the install test file is.

Yup, I am fine with this on an ongoing basis; I don't think it needs a follow-up PR.

---

Reviewed 1 of 13 files at r10, 11 of 12 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

examples/kuka_iiwa_arm/test/kuka_simulation_installed_test.py, line 4 at r11 (raw file):

import os

BTW This blank line is unconventional?

---

tools/install/install.bzl, line 395 at r11 (raw file):

    # Generate test installation script
    if ctx.attr.install_tests_script and not script_tests:
        fail("`install_test_script_template` is not empty but no `script_tests` were provided.")  # noqa

BTW install_test_script_template name is stale?

---

tools/install/test/install_tests.py, line 24 at r11 (raw file):

        sys.stderr.write(str(sys.argv)+str('\n'))
        if len(sys.argv) <= 1:
            return

I am a bit frightened of this -- its a short-circuit that doesn't run any install tests. I guess its used for the lighter-weight //tools/install:install_tests tests (with no cmds), but I feel like it risks having the //:install test lose all its test cases and nobody notices.

I am not actually sure what the purpose of //tools/install:install_tests is. It checks that the installation succeeds without error, but doesn't run any of the installed products? Without more comments, I don't understand why //:install_test on its own isn't good enough. The install is slow to build and to execute, so doing it less often is great (and a major motivator for this PR).

---

tools/install/test/install_tests.py, line 37 at r11 (raw file):

if __name__ == '__main__':
    unittest.main(argv=sys.argv[1:] if len(sys.argv) > 1 else None)

This file only has one assertion (the listdir cross-check). I think perhaps the unittest framework here is doing more harm than good? The qualm being that I don't really understand what this line is trying to do at all.

---

Comments from Reviewable

[jamiesnape]

(modulo CI, open discussions)

---

Reviewed 1 of 13 files at r10, 11 of 12 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[fbudin69500]

Review status: 14 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

examples/kuka_iiwa_arm/test/kuka_simulation_installed_test.py, line 4 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This blank line is unconventional?

Done.

---

tools/install/install.bzl, line 395 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW install_test_script_template name is stale?

Good catch. It was corrected to ctx.attr.install_tests_script.

---

tools/install/test/install_tests.py, line 24 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am a bit frightened of this -- its a short-circuit that doesn't run any install tests. I guess its used for the lighter-weight //tools/install:install_tests tests (with no cmds), but I feel like it risks having the //:install test lose all its test cases and nobody notices.

I am not actually sure what the purpose of //tools/install:install_tests is. It checks that the installation succeeds without error, but doesn't run any of the installed products? Without more comments, I don't understand why //:install_test on its own isn't good enough. The install is slow to build and to execute, so doing it less often is great (and a major motivator for this PR).

The purpose of //tools/install:install_tests was to verify that the script itself was working, before being modified, since in the previous versions of the patch it was used as a template that was customized. Now that the script is not a template anymore, I will remove this test and simplify the script to avoid this complicated situation.

---

tools/install/test/install_tests.py, line 37 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This file only has one assertion (the listdir cross-check). I think perhaps the unittest framework here is doing more harm than good? The qualm being that I don't really understand what this line is trying to do at all.

I simplified this test.

---

Comments from Reviewable

[jamiesnape]

Reviewed 1 of 13 files at r10, 7 of 7 files at r12.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

LGTM.

+@ggould-tri for platform review per schedule, please.

---

Reviewed 1 of 13 files at r10, 7 of 7 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[ggould-tri]

pending minor discussions below.

---

Reviewed 1 of 19 files at r7, 4 of 10 files at r8, 4 of 13 files at r11, 6 of 8 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

---

common/test/resource_tool_installed_test.py, line 63 at r12 (raw file):

            self.assertEqual(data.read(), resource_data)

        # Remove environment variable.

This comment isn't really intuitive about what's going on. Better "use --add_resource_search_path instead of environment variable".

---

examples/kuka_iiwa_arm/test/iiwa_wsg_simulation_installed_test.py, line 14 at r12 (raw file):

        # Get install directory.
        install_dir = install_test_helper.get_install_dir()
        # Make sure the simulation can run without error.  We set cwd="/" to

Since you don't visibly set cwd="/" anywhere here, but it is instead set in check_call (as with nearly every other test), perhaps reword or relocate this comment?

---

examples/kuka_iiwa_arm/test/kuka_simulation_installed_test.py, line 13 at r12 (raw file):

        # Get install directory
        install_dir = install_test_helper.get_install_dir()
        # Make sure the simulation can run without error.  We set cwd="/" to

Same comment as above except here you actually deleted the relevant cwd="/".

---

tools/install/install.bzl, line 282 at r12 (raw file):

#------------------------------------------------------------------------------
# Compute install test actions.

BTW This comment just recites the function name and adds no information. Mention the return value and/or parameters, or omit it?

---

tools/install/install.bzl, line 399 at r12 (raw file):

        ctx.actions.write(
            output = ctx.outputs.install_tests_script,
            content = "  # noqa\n".join(script_tests),

For some reason I find this line completely hilarious.

---

tools/install/install_test_helper.py, line 101 at r12 (raw file):

        ret = proc.poll()
        assert ret is None
    # time's up: kill the proc (and it's group):

FYI Capitalize.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

tools/install/install.bzl, line 399 at r12 (raw file):

Previously, ggould-tri wrote…

For some reason I find this line completely hilarious.

Actually, I think this is vestigial and should be removed now?

---

Comments from Reviewable

[fbudin69500]

Review status: 14 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

common/test/resource_tool_installed_test.py, line 63 at r12 (raw file):

Previously, ggould-tri wrote…

This comment isn't really intuitive about what's going on. Better "use --add_resource_search_path instead of environment variable".

Done.

---

examples/kuka_iiwa_arm/test/iiwa_wsg_simulation_installed_test.py, line 14 at r12 (raw file):

Previously, ggould-tri wrote…

Since you don't visibly set cwd="/" anywhere here, but it is instead set in check_call (as with nearly every other test), perhaps reword or relocate this comment?

Done.

---

examples/kuka_iiwa_arm/test/kuka_simulation_installed_test.py, line 13 at r12 (raw file):

Previously, ggould-tri wrote…

Same comment as above except here you actually deleted the relevant cwd="/".

Done.

---

tools/install/install.bzl, line 282 at r12 (raw file):

Previously, ggould-tri wrote…

BTW This comment just recites the function name and adds no information. Mention the return value and/or parameters, or omit it?

Done.

---

tools/install/install.bzl, line 399 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Actually, I think this is vestigial and should be removed now?

# noqa has been removed and install_test.py has been updated accordingly.

---

tools/install/install_test_helper.py, line 101 at r12 (raw file):

Previously, ggould-tri wrote…

FYI Capitalize.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 6 of 6 files at r13.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[ggould-tri]

Reviewed 1 of 19 files at r7, 1 of 10 files at r8, 1 of 13 files at r10, 1 of 12 files at r11, 6 of 6 files at r13.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jamiesnape]

Reviewed 6 of 6 files at r13.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 13 of 13 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jamiesnape]

@jwnimmer-tri We are just waiting on the official Reviewable stamp of approval from you. Anything else needed here?

[jwnimmer-tri]

sorry.