Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

[noetic] wrap rostest call to add python pointing to sys.executable in PATH#1879

Merged
dirk-thomas merged 5 commits intoros:noetic-develfrom
sloretz:rostest_custom_python
Feb 21, 2020
Merged

[noetic] wrap rostest call to add python pointing to sys.executable in PATH#1879
dirk-thomas merged 5 commits intoros:noetic-develfrom
sloretz:rostest_custom_python

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Feb 7, 2020

Another possible solution to #1830. This adds a temporary directory with a symlink to python to the path variable.

This uses tempfile.TemporaryDirectory which is only available in python 3.2 and above, so it must not be merged onto the melodic-devel branch. With this PR (and #1870) I don't see any ros_comm tests using Python 2, though many tests raise ModuleNotFoundError when trying to import other packages.

@sloretz
Wrap rostest call to set python in PATH
c2d6f14 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz requested a review from dirk-thomas February 7, 2020 17:59
@sloretz sloretz self-assigned this Feb 7, 2020
@dirk-thomas dirk-thomas changed the title Wrap rostest call to set python in PATH [noetic] wrap rostest call to add python pointing to sys.executable in PATH Feb 7, 2020
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Feb 7, 2020

though many tests raise ModuleNotFoundError when trying to import other packages.

Can you please reference some of the failing tests here to be able to determine what might cause this.

dirk-thomas
dirk-thomas reviewed Feb 7, 2020
View reviewed changes
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 7, 2020

Can you please reference some of the failing tests here to be able to determine what might cause this.

======================================================================
ERROR: test_validate_master_launch (unit.test_roslaunch_launch.TestRoslaunchLaunch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sloretz/ws/noetic/src/ros_comm/tools/roslaunch/test/unit/test_roslaunch_launch.py", line 47, in test_validate_master_launch
    import roslaunch.launch
  File "/home/sloretz/ws/noetic/src/ros_comm/tools/roslaunch/src/roslaunch/__init__.py", line 62, in <module>
    from .scriptapi import ROSLaunch
  File "/home/sloretz/ws/noetic/src/ros_comm/tools/roslaunch/src/roslaunch/scriptapi.py", line 42, in <module>
    import roslaunch.parent
  File "/home/sloretz/ws/noetic/src/ros_comm/tools/roslaunch/src/roslaunch/parent.py", line 54, in <module>
    import roslaunch.server
  File "/home/sloretz/ws/noetic/src/ros_comm/tools/roslaunch/src/roslaunch/server.py", line 79, in <module>
    from rosgraph_msgs.msg import Log
ModuleNotFoundError: No module named 'rosgraph_msgs'

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Feb 7, 2020 •
edited
Loading

That test (roslaunch/test/unit/test_roslaunch_launch.py) isn't run by rostest but nose, right? So this change wouldn't have any effect.

@sloretz
Consolidate skip logic
2a1aa83 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
sys.executable is always an absolute path
6e57fa0 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Fix skip
c462a96 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 7, 2020

That test (roslaunch/test/unit/test_roslaunch_launch.py) isn't run by rostest but nose, right? So this change wouldn't have any effect.

Yeah it seems unrelated to this PR. rosgraph_msgs doesn't appear to be part of the underlay when building with catkin_make_isolated. No clue why yet, but I'll move that to a separate issue.

@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Feb 7, 2020

rosgraph_msgs doesn't appear to be part of the underlay when building with catkin_make_isolated.

I am surprised since it is a declared dependency:

ros_comm/test/test_rosbag/package.xml

Line 35 in aedcece

<build_depend>rosgraph_msgs</build_depend>

dirk-thomas
dirk-thomas approved these changes Feb 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

lgtm

@sloretz sloretz mentioned this pull request Feb 14, 2020
Closed
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 18, 2020 •
edited
Loading

@ros-pull-request-builder retest this please

edit: test failures, rerunning to check for flakiness, these are the old tests

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 19, 2020

@ros-pull-request-builder retest this please

dirk-thomas
dirk-thomas approved these changes Feb 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Just waiting for the noetic-devel branch to be created.

@dirk-thomas dirk-thomas mentioned this pull request Feb 20, 2020
Merged
@dirk-thomas dirk-thomas changed the base branch from melodic-devel to noetic-devel February 21, 2020 00:30
@dirk-thomas dirk-thomas merged commit 34a4335 into ros:noetic-devel Feb 21, 2020
@dirk-thomas dirk-thomas mentioned this pull request Feb 24, 2020
Merged
@sloretz sloretz mentioned this pull request Mar 10, 2020
Closed
@dirk-thomas dirk-thomas mentioned this pull request Mar 20, 2020
Merged
@k-okada k-okada mentioned this pull request Oct 30, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sloretz sloretz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sloretz @dirk-thomas