Skip to content

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

Merged
clalancette merged 1 commit intomasterfrom
clalancette/add-pytest-ini
Jun 19, 2020
Merged

Add pytest.ini so local tests don't display warning#224
clalancette merged 1 commit intomasterfrom
clalancette/add-pytest-ini

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

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

See ros2/ros2#951 for more details and CI.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Copy link
Copy Markdown
Member

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Thanks @clalancette, looks good to me.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 18, 2020

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
Copy link
Copy Markdown
Member

Thanks @clalancette.

2 things seem surprising though:

  • 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?
  • 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?

@clalancette
Copy link
Copy Markdown
Contributor Author

* 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 ros2/ci#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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

@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
Copy link
Copy Markdown
Member

mikaelarguedas commented Jun 18, 2020

@dirk-thomas thanks, that must be why 👍, as of ros2/ros2#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)

Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

+1, needed if installing pytest from pip

@clalancette
Copy link
Copy Markdown
Contributor Author

Checks are green, CI in ros2/ros2#951 was green, approved. Merging, thanks!

@clalancette clalancette merged commit 3b6d145 into master Jun 19, 2020
@clalancette clalancette deleted the clalancette/add-pytest-ini branch June 19, 2020 12:22
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.

4 participants