-
Notifications
You must be signed in to change notification settings - Fork 129
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
Enable support for code coverage #504
base: master
Are you sure you want to change the base?
Conversation
3f5153e
to
1c6bfce
Compare
The following tests are conducted on packml_ros2, which is a ROS2 meta package with
Badges are redirected to builds and reports There is a difference in how coverage.py is calculated between coveralls.io and codecov.io, I compared the coverage line by line there is no difference.
|
Hi @ipa-mdl or anyone with write access Test cases: Since the packages that I tested on are both not fully merged yet, I would like some recommendation regarding the packages to do wrap_test on ideally is
There are still some more TODOs, I will address in another PR
|
This is really nice. Thank you for doing this. It has been on a long list of things I've wanted to do and your implementation covers more use cases and more features that I would have. |
If you have extra bandwidth on this one more nice feature would be coverage percentage output on local runs ( |
Thank you @tylerjw!! At the moment, a display like the following will still be printed out when CODE_COVERAGE is set to something else, e.g. true or term (that is not coveralls.io or codecov.io). I don't think you will have problem getting these output using
Unfortunately, the output are not combined into one, Instead of Update: I opened a ticket colcon/colcon-core#359 regarding this to make a combined coverage, but I think what we have now is good for most packages and implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all: very impressive!
There are some bigger and smaller issues, which need to be addressed before we can merge this.
For now I'd like to focus on the biggest..
This PR contains two parts: report generation and report uploading.
The generation part (including the first half of upload_coverage_report
) looks file so far, but the upload is hard to maintain this way..
Instead of uploading everything during the build, it would be easier to do it outside of Docker.
The upload from within the build might even create problems with local testing.
Please try to move the upload handling out of the build.
Some CI services might support this out of the box anyway, including the handling of credentials.
f4849ff
to
b703795
Compare
Thanks @ipa-mdl
So now I am exposing the report files onto industrial_ci/industrial_ci/src/docker.sh Lines 76 to 79 in 5b0bbc7
The output files are respectively
It seems to me, to upload report, the only way currently is to put them at the end of the entrypoint. Lines 45 to 47 in 5b0bbc7
CODE_COVERAGE can also be set to anything else like true or term to only expose the report files and allow users to decide how to deal with the coverage report, e.g. using existing coverage functionality from Github Action Apps or Gitlab artifact. Lastly, thanks to @rarrais 's work on ros_coverage. I have added additional improvement to support ROS1 python2 and python3 packages with unittests or nosetests on catkin_tools and colcon, by setting CATKIN_WITH_COVERAGE env variable to 1. (Note: This doesn't work on catkin_make and catkin_make_isolated) |
Update: Now working see https://codecov.io/gh/agutenkunst/minimal_test_package/tree/b362893734dfa87fc738b51fbd0f0fcf17b695e9/src Hi @Briancbn nice work! Looking forward to this! I setup https://github.com/agutenkunst/minimal_test_package in order to check this (using ROS1 gtest + nosetest) It failed with
see https://travis-ci.com/github/agutenkunst/minimal_test_package/builds/174644770 Could very well be my mistake. Any ideas? |
Ok coveralls.io fails with
see https://travis-ci.com/github/agutenkunst/minimal_test_package/builds/174654480 Did you experience somethink like this before? |
Could you provide your insights on this? |
Thanks so much @agutenkunst for taking the time to test this PR:)
This is due to not enabling coveralls.io for your package.
Great! How did you resolve the problem?
So Update: |
So currently codecov.io works (https://codecov.io/gh/agutenkunst/minimal_test_package/tree/b362893734dfa87fc738b51fbd0f0fcf17b695e9/src) (seemed to fail just once and worked on restarting the job, but since I clone your fork by branch name and not commit it is hard to recap) coveralls.io
even after I enabled the project on coveralls.io see https://travis-ci.com/github/agutenkunst/minimal_test_package/builds/174654480 for full output. |
Sorry about misleading earlier, I have found out that this is due to the Travis linux distro is The xenial images by default also only provide python2, which might be a problem in the future. However, I am also reluctant to put in extra python3 installation script inside the install tag. Update: https://urllib3.readthedocs.io/en/latest/user-guide.html#ssl-py2 This is the solution to that warning. I will see whether adding it in will break anything. |
I have moved the upload script to somewhere after docker_commit. I think this is be better than ci_main.sh and outside of ci_main.sh shell.
|
Is there any way I can help with this. I think this is one of the last things we need before we can use industrial_ci for moveit on github actions. |
I tried it and wasn't able to get it to work for me. However I think that might be because I'm using BASEDIR now and that feature was merged later (and is probably part of the conflicts). One thing I was wondering is does this PR do the filtering out of test files? How about if someone gives a repo file for the TARGET_WORKSPACE, will it filter the results to just the repo being tested. Here is the script I wrote that I'm using for now to prepare my coverage report until I can use this PR: #!/bin/bash
pushd $BASEDIR
BLUE='\033[0;34m'
NOCOLOR='\033[0m'
echo -e "${BLUE}Capture coverage info${NOCOLOR}"
lcov --capture --directory target_ws --output-file coverage.info
echo -e "${BLUE}Extract repository files${NOCOLOR}"
lcov --extract coverage.info "$BASEDIR/target_ws/src/$TARGET_REPO_NAME/*" --output-file coverage.info
echo -e "${BLUE}Filter out test files${NOCOLOR}"
lcov --remove coverage.info '*/test/*' --output-file coverage.info
echo -e "${BLUE}Output coverage data for debugging${NOCOLOR}"
lcov --list coverage.info
popd |
Hi @tylerjw, sorry I will only be able to follow up on this after March ends,..qaq. I can add you as collaborator if you would like to contribute to this PR?
It does filter out the test files for both python and C++
No, it doesn't. In fact, if you don't collect the initial zero report, files that hasn't been tested (no report) will not be included in the coverage -- which technically is an overestimation of the coverage data.
I have another branch to address non-conventional ignore of both relative and absolute paths. Briancbn#1 (comment) will try to include it after I have some more time. |
00063ee
to
a06d5d1
Compare
f87a92b
to
70cb58b
Compare
Signed-off-by: Chen Bainian <[email protected]> Make python coverage only available to colcon in ROS2 Signed-off-by: Chen Bainian <[email protected]> Revert mercurial setup changes Signed-off-by: Chen Bainian <[email protected]> Fix curl not found in ROS1 Signed-off-by: Chen Bainian <[email protected]> temp Signed-off-by: Chen Bainian <[email protected]> Add lcov and find report command Signed-off-by: Chen Bainian <[email protected]> Add in coveralls.io option Signed-off-by: Chen Bainian <[email protected]> Fix .coverage not found Signed-off-by: Chen Bainian <[email protected]> Fix coveralls not found Signed-off-by: Chen Bainian <[email protected]> Fix wrong .coverage path Signed-off-by: Chen Bainian <[email protected]> Use coveralls-merge Signed-off-by: Chen Bainian <[email protected]> Remove psutils installation Signed-off-by: Chen Bainian <[email protected]> Use coveragepy combine report Signed-off-by: Chen Bainian <[email protected]> Use coveragerc for files ignore Signed-off-by: Chen Bainian <[email protected]> Use coveralls specific environment variables Signed-off-by: Chen Bainian <[email protected]> Avoid exit for pure c++ or python package Signed-off-by: Chen Bainian <[email protected]> Avoid coveragepy report error Signed-off-by: Chen Bainian <[email protected]> Use target repo name Signed-off-by: Chen Bainian <[email protected]> Force Debug build during coverage build Signed-off-by: Chen Bainian <[email protected]> Expose .ici_coverage_report for coverage reports Signed-off-by: Chen Bainian <[email protected]> Run cd in subshell and expose coverage report files Signed-off-by: Chen Bainian <[email protected]> Some sed magic to remove identifiable absolute path Signed-off-by: Chen Bainian <[email protected]> Add support for ROS1 python unittest, Credit: @rarrais Signed-off-by: Chen Bainian <[email protected]> Move coveralls report merging inside, expose only merged json file Signed-off-by: Chen Bainian <[email protected]> Use CODECOV env variable directly in function Signed-off-by: Chen Bainian <[email protected]> Sample travis report upload script Signed-off-by: Chen Bainian <[email protected]> Fix typo on sample travis report upload script Signed-off-by: Chen Bainian <[email protected]> Remove redundant files Signed-off-by: Chen Bainian <[email protected]> Display coveragepy installation elegantly Signed-off-by: Chen Bainian <[email protected]> Add Gitlab support Signed-off-by: Chen Bainian <[email protected]> Fix some wrong versions of python Signed-off-by: Chen Bainian <[email protected]> Add Github Action coverage support Signed-off-by: Chen Bainian <[email protected]> Upgrade pip first Signed-off-by: Chen Bainian <[email protected]> Python report ignore everything from build and devel Signed-off-by: Chen Bainian <[email protected]> Wrong! Ignore devel instead Signed-off-by: Chen Bainian <[email protected]> Python ignore everything from build and devel Signed-off-by: Chen Bainian <[email protected]> Use include pattern instead of omit Signed-off-by: Chen Bainian <[email protected]> Update author copyrights Signed-off-by: Chen Bainian <[email protected]> Move coverage helper function into coverage.sh Signed-off-by: Chen Bainian <[email protected]> Streamline coverage upload script Signed-off-by: Chen Bainian <[email protected]> Abstract python pip installation install setup tool as well Move code coverage inside docker script
Signed-off-by: Chen Bainian <[email protected]>
This branch seems stale. Is there any way I can add the code coverage to my ros industrial Github action? |
Feature description
This PR aims to achieve what is described in #500 including
generating code coverage report for the following builder and packages
code coverage report tools support
CI
additionally,
How to use
First, please register in codecov.io or coveralls.io using your respective git account. Then you need to enable inside your codecov.io and coveralls.io account to accept reports from specific repository.
Configure CODE_COVERAGE variable to the name of the web reporting tool to enable code coverage generation and uploading report to the respective web reporting tool website,
You can apply to all of your jobs by something like below, Or define
CODE_COVERAGE
per job.In Travis CI
In Gitlab CI
In Github Action
Known Issues
ROS1 Python Coveralls: Cannot ignore__init__.py
file in thebuild/
directoryUpdates
July 2: Add more detailed instructions for Gitlab CI and Github Action
July 2: Add Known Issues
July 8: Add codecov and coveralls setup instructions
July 8: Ignore python reports on files inside build/ and devel/