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.

Sign up for GitHub

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

Jump to bottom

Mention other platforms in 'pytest/pytest-cov not found' warning #337

Merged
merged 2 commits into from
May 11, 2021
Merged

Mention other platforms in 'pytest/pytest-cov not found' warning #337

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e26dd0d
Mention other platforms in 'pytest/pytest-cov not found' warning
christophebedard May 10, 2021
0752f31
Change 'Ubuntu/Debian' to 'Linux'
christophebedard May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ament_cmake_pytest/cmake/ament_add_pytest_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ function(ament_add_pytest_test testname path)
ament_has_pytest(has_pytest QUIET PYTHON_EXECUTABLE "${ARG_PYTHON_EXECUTABLE}")
if(NOT has_pytest)
message(WARNING
"The Python module 'pytest' was not found, pytests can not be run (e.g. "
"on Ubuntu/Debian install the package 'python3-pytest')")
"The Python module 'pytest' was not found, pytests cannot be run. "
"On Ubuntu/Debian, install the 'python3-pytest' package. "
"On other platforms, install 'pytest' using pip.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes more sense to just point to the generic installation docs instead. That is, something like:

      "The Python module 'pytest' was not found, pytests cannot be run. "
      "See https://docs.ros.org/en/rolling/Installation.html to install pytest for your platform.")

This has the downside that we are embedding a rolling URL in the code, but it avoids duplicating the install instructions. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would make sense for pytest because it is part of a normal ROS 2 installation (however, pytest-cov isn't), but besides embedding rolling in the URL, I kind of prefer the current warning messages because they tell you exactly what you need to do (install ___ package) instead of pointing you to the generic installation docs just to install a Python package or two.

In any case, I can switch to that for pytest if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another thing: should ament really link to ROS 2 like that? As opposed to the other way around, ROS 2 linking to ament.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of prefer the current warning messages because they tell you exactly what you need to do (install ___ package) instead of pointing you to the generic installation docs just to install a Python package or two.

Yeah, that's true.

another thing: should ament really link to ROS 2 like that? As opposed to the other way around, ROS 2 linking to ament.

In theory, no. In practice, it seems unlikely that anyone outside the ROS 2 ecosystem is using ament (at least, I don't know of any). It would be nice to be proven wrong, though :).

Overall, I agree with you about keeping the current message. The one thing I would say is let's change the message to say "On Linux, install the 'python3-pytest' package", since that will also cover RHEL/Fedora.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, no. In practice, it seems unlikely that anyone outside the ROS 2 ecosystem is using ament (at least, I don't know of any). It would be nice to be proven wrong, though :).

maybe one day! 😄

Overall, I agree with you about keeping the current message. The one thing I would say is let's change the message to say "On Linux, install the 'python3-pytest' package", since that will also cover RHEL/Fedora.

done: 0752f31

return()
endif()

Expand Down Expand Up @@ -115,8 +116,9 @@ function(ament_add_pytest_test testname path)
)
if(NOT pytest_cov_version)
message(WARNING
"The Python module 'pytest-cov' was not found, test coverage will not be produced "
"(e.g. on Ubuntu/Debian install the package 'python3-pytest-cov')")
"The Python module 'pytest-cov' was not found, test coverage will not be produced. "
"On Ubuntu/Debian, install the 'python3-pytest-cov' package. "
"On other platforms, install 'pytest-cov' using pip.")
else()
set(coverage_directory "${CMAKE_CURRENT_BINARY_DIR}/pytest_cov/${testname}")
file(MAKE_DIRECTORY "${coverage_directory}")
Expand Down