ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
90 stars 45 forks source link

Add pytest.ini so local tests don't display warning #224

Closed clalancette closed 4 years ago

clalancette commented 4 years ago

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

See https://github.com/ros2/ros2/issues/951 for more details and CI.

codecov[bot] commented 4 years ago

Codecov Report

Merging #224 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   78.02%   78.02%           
=======================================
  Files          16       16           
  Lines         578      578           
  Branches       51       51           
=======================================
  Hits          451      451           
  Misses        107      107           
  Partials       20       20           
Flag Coverage Δ
#unittests 78.02% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8263f68...bf8723e. Read the comment docs.

mikaelarguedas commented 4 years ago

Thanks @clalancette.

2 things seem surprising though:

clalancette commented 4 years ago
* we don't see any of these warnings on the nightly CI of this repo. Is this change really necessary? if so how can we reproduce it here?

Yeah, you don't see it in nightly CI because of this piece of code: https://github.com/ros2/ci/blob/8c758cf359ec5328f652f5f068654aa6101b44a7/ros2_batch_job/__main__.py#L384 . However, we'd like to remove that code (see https://github.com/ros2/ci/pull/471), and if you run the tests locally, you always get a warning. Thus it seems like the right way to fix both problems is to put these pytest.ini files where they are needed.

* you mention that the issue is being worked around on the master branch of ROS 2 CI already and the CI jobs you link to use that same `master` branch, is there a way to confirm that this PR solves the problem you're facing there?

The easiest way is to run the tests locally with colcon test --packages-select sros2; you'll see the warnings before this patch, and then won't see them after. I've confirmed that locally. I'll also eventually run a CI job with the workaround in ros2/ci removed, but I don't really think it is a prerequisite here.

mikaelarguedas commented 4 years ago

Yeah, you don't see it in nightly CI because of this piece of code:

I'm referring to the nightly CI on this repository that don't use ros2/ci : https://github.com/ros2/sros2/actions?query=workflow%3A%22SROS2+CI%22+branch%3Amaster. For example the last one an hour ago: https://github.com/ros2/sros2/actions/runs/140086980 I cant find the warning on either the one based on the nightly archive or the one building all from source.

The easiest way is to run the tests locally with colcon test --packages-select sros2; you'll see the warnings before this patch, and then won't see them after. I've confirmed that locally. I'll also eventually run a CI job with the workaround in ros2/ci removed, but I don't really think it is a prerequisite here.

Yeah that's why I'm asking, I just tried that and couldn't reproduce:

docker run -it --rm osrf/ros2:nightly
cd && mkdir src
git clone https://github.com/ros2/sros2 src/sros2
colcon build --packages-select sros2 --event-handlers console_direct+
colcon test --packages-select sros2 --event-handlers console_direct+
...
--------------- generated xml file: /root/build/sros2/pytest.xml ---------------
========================== 28 passed in 2.58 seconds ==========================
grep -r Depreca | wc -l
0
dirk-thomas commented 4 years ago

@mikaelarguedas Since the referenced actions are a custom CI solution you probably know best where and how they diverge from the install instructions. E.g. if they install pytest from pip as described here: https://index.ros.org/doc/ros2/Installation/Foxy/Linux-Development-Setup/#install-development-tools-and-ros-tools

mikaelarguedas commented 4 years ago

@dirk-thomas thanks, that must be why :+1:, as of https://github.com/ros2/ros2/issues/722 the docker images rely on colcon to provide pytest instead of installing potentially incompatible versions.

So currently the recommended way (from the install instructions) is to install first pytest and some pytest plugins from apt and then from pip, is that correct ? (if so we can update the docker images accordingly)

clalancette commented 4 years ago

Checks are green, CI in https://github.com/ros2/ros2/issues/951 was green, approved. Merging, thanks!