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

test_utilities: Teach drake_py_unittest where the source lives #10969

Merged
merged 1 commit into from Mar 20, 2019

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Mar 20, 2019

Previously, we were searching in "." for the source code. That is dangerous when running a test manually from bazel-bin (instead of via bazel test), and is ambiguous for reused basenames like lcm_test.

Without this PR, running bazel-bin/bindings/pydrake/py/lcm_test by hand fails.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@EricCousineau-TRI for feature review, please.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

At some point, it would be nicer to make this extremely explicit - drake_py_unittest should have some logic to scribble in the main path to the Bazel invocation script. But eh, that's a relatively non-functional change for high-effort.

Thanks!

Reviewed 1 of 1 files at r1.
Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri], labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

CI found a few corner cases. I'll work on it.

Previously, we were searching in "." for the source code.  That is
dangeous when running a test manually from bazel-bin (instead of via
bazel test), and is ambiguous for reused basenames like lcm_test.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

As it turns out, the prior find logic was "getting lucky" on the install_test. By making the rules more rigid, I had to make the install_test more explicit. PTAL.

Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri], labeled "do not merge" (waiting on @EricCousineau-TRI)

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2.
Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri], labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

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

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: all discussions resolved, platform LGTM from [ericcousineau-tri, ggould-tri], labeled "do not merge"

@jwnimmer-tri jwnimmer-tri merged commit 040fc71 into RobotLocomotion:master Mar 20, 2019
@jwnimmer-tri jwnimmer-tri deleted the py-unittest-main branch March 20, 2019 19:22
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.

None yet

3 participants