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

Add downstream Python test. #85

Merged

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Jan 19, 2018

Currently the resource in question is broken.

Relates #9


This change is Reviewable

@EricCousineau-TRI EricCousineau-TRI requested review from fbudin69500 and removed request for fbudin69500 January 19, 2018 17:40
@EricCousineau-TRI
Copy link
Collaborator Author

+@fbudin69500 for feature review, please (if you have time).


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


Comments from Reviewable

@jwnimmer-tri
Copy link
Contributor

FYI I am fine with having Shambhala have as many tests as we want, but in general we should have the Drake self-tests catch things like this pre-merge to the extent practical.

@EricCousineau-TRI
Copy link
Collaborator Author

Sounds good!

It'd still be nice, though, if all unittests were run on CI (e.g. ensuring that GTest is always found on CI, which may have not been the case?)
See #86 #87

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Feb 8, 2018

Updated since RobotLocomotion/drake#7809 is fixed.

@jamiesnape
Copy link
Contributor

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


drake_cmake_installed/CMakeLists.txt, line 44 at r1 (raw file):

set(_python_version 2.7)
find_package(PythonInterp ${_python_version} MODULE REQUIRED)

find_package(PythonInterp 2.7 EXACT MODULE REQUIRED)


drake_cmake_installed/CMakeLists.txt, line 46 at r1 (raw file):

find_package(PythonInterp ${_python_version} MODULE REQUIRED)
set(_drake_sitepackages
    "${drake_DIR}/../../python${_python_version}/site-packages")

${drake_DIR}/../../python${PYTHON_VERSION_MAJOR }.${PYTHON_VERSION_MINOR}/site-packages, but create an issue to define a variable something DRAKE_PYDRAKE_DIR in drake.cps (I forget the naming convention we have used so far).


drake_cmake_installed/CMakeLists.txt, line 48 at r1 (raw file):

    "${drake_DIR}/../../python${_python_version}/site-packages")
get_filename_component(_drake_sitepackages "${_drake_sitepackages}" ABSOLUTE)
# TODO(eric.cousineau): Make this a `pydrake` package, than can be depended on,

This functionality does exist in CMake, so not an actionable TODO unless you want us to add the functionality and move to a newer CMake (which I doubt you do just for this).


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 45 at r1 (raw file):

add_test(
    NAME test_find_resource_py
    COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/test_find_resource.py)

${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/test_find_resource.py


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 49 at r1 (raw file):

    test_find_resource_py
    PROPERTIES
        ENVIRONMENT PYTHONPATH=$ENV{PYTHONPATH})

"PYTHONPATH=$ENV{PYTHONPATH}"


drake_cmake_installed/src/find_resource/test_find_resource.py, line 1 at r1 (raw file):

#!/usr/bin/env python

We don't need a shebang. Replace with

# -*- mode: python -*-
# vi: set ft= python :

drake_cmake_installed/src/find_resource/test_find_resource.py, line 6 at r1 (raw file):

import pydrake
from pydrake import common

Do we need all these imports?


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions.


drake_cmake_installed/CMakeLists.txt, line 44 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

find_package(PythonInterp 2.7 EXACT MODULE REQUIRED)

Done.


drake_cmake_installed/CMakeLists.txt, line 46 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

${drake_DIR}/../../python${PYTHON_VERSION_MAJOR }.${PYTHON_VERSION_MINOR}/site-packages, but create an issue to define a variable something DRAKE_PYDRAKE_DIR in drake.cps (I forget the naming convention we have used so far).

Done.


drake_cmake_installed/CMakeLists.txt, line 48 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

This functionality does exist in CMake, so not an actionable TODO unless you want us to add the functionality and move to a newer CMake (which I doubt you do just for this).

Done.


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 45 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/test_find_resource.py

I see no difference - can I ask what the defect is? (added a linebreak for wrapping)


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 49 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

"PYTHONPATH=$ENV{PYTHONPATH}"

Done.


drake_cmake_installed/src/find_resource/test_find_resource.py, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

We don't need a shebang. Replace with

# -*- mode: python -*-
# vi: set ft= python :

Done.


drake_cmake_installed/src/find_resource/test_find_resource.py, line 6 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Do we need all these imports?

Done.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 45 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

I see no difference - can I ask what the defect is? (added a linebreak for wrapping)

Sorry, I meant to add double quotes.

"${PYTHON_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/test_find_resource.py"



---


*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake-shambhala/85#-:-L4sDk-SuZpPPtGyd6mN:bnfp4nl)*
<!-- Sent from Reviewable.io -->

@EricCousineau-TRI
Copy link
Collaborator Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


drake_cmake_installed/src/find_resource/CMakeLists.txt, line 45 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Sorry, I meant to add double quotes.

"${PYTHON_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/test_find_resource.py"

Done.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

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


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI merged commit fc30783 into RobotLocomotion:master Feb 9, 2018
@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants