add codecov.io support

[tylerjw]

This adds codecov.io report uploading support. As noted in the README.md change in order to use these changes a cmake INTERFACE library needs to be added to the build instructions to enable coverage report generation on only your project's files. It might work to instead add these options through CMAKE_ARGS but then it'll generate a coverage report for all of the code in your project (including tests). There is a way to prune these with a codecov.yaml file. Let me know if you would rather me implement it this way.

The copying of the build directory out of docker allows the codecov.io script to find the artifacts it needs to compile the report. I tried running the codecov.io script inside of the docker image but that didn't work because it couldn't reach codecov.io. I don't understand networking in docker images enough to troubleshoot this.

Thank you for reviewing this.

[davetcoleman]

Did quick scan while at ROSCon, this is beautiful !

[henningkayser]

I'm wondering what this would imply for the current effort to get CircleCi+Codecov running for MoveIt 2: moveit/moveit2#44

I tried running the codecov.io script inside of the docker image but that didn't work because it couldn't reach codecov.io. I don't understand networking in docker images enough to troubleshoot this.

You mean "curl -s https://codecov.io/bash" doesn't work for querying the script? That's odd, we use wget and curl for different purposes all the time within docker. Uploading a report is not possible from within a nested docker container right away. Passing the environment variables to the docker run command doesn't work? https://docs.codecov.io/docs/testing-with-docker.

[tylerjw]

You mean "curl -s https://codecov.io/bash" doesn't work for querying the script? That's odd, we use wget and curl for different purposes all the time within docker. Uploading a report is not possible from within a nested docker container right away. Passing the environment variables to the docker run command doesn't work? https://docs.codecov.io/docs/testing-with-docker.

Curl works just fine. It is the uploading that fails. It fails because it can't reach codecov.io to send the report.

[rhaschke]

Thanks for this great contribution! Apart from some minor comments (see below), I don't like

1. that you need to copy the build folder. See Henning's comment add codecov.io support #91 (review) for a possible solution.
2. that we need to augment the project's cmake files. If you add the --coverage option globally, I guess a coverage report is generated for all packages. Manually listing them in codecov.yaml for pruning is error-prone as well. But you could just delete all corresponding files not belonging to packages that are actually tested. We do similar things for blacklisting those packages for unit testing.

Out of curiosity: Where are the coverage reports generated? Does adding the option --coverage suffices to generate those reports on the execution of binaries? Don't you need to run gcov as well, or is this done by the curl-downloaded bash script?

EDIT: Just looked into the curl script: Indeed, it runs gcov. You can ignore paths by passing -g GLOB to this script.

[tylerjw]

@rhaschke Thank you for the review. I am responding to give you an update on this and respond to your specific concerns and get your feedback on the approach I'm taking.

1. that you need to copy the build folder. See Henning's comment #91 (review) for a possible solution.

This works, I've got this working on my repo I'm using for testing and verifying this change and will include this in my next commit for this.

2. that we need to augment the project's cmake files. If you add the --coverage option globally, I guess a coverage report is generated for all packages. Manually listing them in codecov.yaml for pruning is error-prone as well. But you could just delete all corresponding files not belonging to packages that are actually tested. We do similar things for blacklisting those packages for unit testing.

I haven't found a way to do this without editing cmake files, however I have found a more robust solution. In that repo there is a Coverage.cmake file that can be included with include(Coverage). This checks for compiler support before changing the compiler flags and allows you to enable it with -DCMAKE_BUILD_TYPE=Coverage". Then you have to exclude folders and files using the codecov.yaml file. However the syntax for it is nice enough:

ignore:
  - "test"
  - "external"
  - "*_main.cpp"

What are your thoughts on this revised approach? Once I get it working on my test repo I will push a commit with these changes.

Edit:
In order for cmake to find the Coverage.cmake file I had to set CMAKE_MODULE_PATH. Do you know if catkin sets this to some default value in projects or if I will also have to set this in each cmake file I edit. One of the goals for this is to get it turned on for the moveit project but I'm having trouble figuring out the best approach to setting the build flags and reducing the size of the required change for the moveit project. Please advise.

[rhaschke]

Dear Tyler,

I think I now better understand how coverage works: Code that shouldn't be considered for coverage (testing source code and external stuff), shouldn't be compiled with instrumentation flags. Hence, your initial approach to define an interface library target that can be "linked" with appropriated build items, seems to be most reasonable.
Obviously we cannot really avoid touching the cmake files. If you want to keep the Coverage.cmake file, for MoveIt it might be useful to provide that file once in moveit_core and re-use it across all the other packages.

[tylerjw]

Hello Robert,

I just pushed an updated version of this. I'm now going to now attempt to get this working with the moveit project to verify my approach. After experimenting with both I think the .codecov.yml file exclude list is easier to understand (and therefore easier for people who maintain/contribute to a project to use) than any approach I derived using cmake file changes to exclude files. Please don't spend time reviewing these changes until I've validated it working with moveit as I might need to change my approach again here to make the changes to moveit to be as simple as possible.

Thank you,
Tyler

[rhaschke]

Just a few comments. Just have seen that you asked not to review this.
Please indicate WIP in the title of the PR in such cases.

[tylerjw]

@rhaschke Today I started trying to get this to work for moveit. Locally I can never get catkin build run_tests to finish. I pushed this up to see if it would finish and travis fails at the same point that my laptop does. It always times out with this message:

The command "catkin build --catkin-make-args run_tests -- --no-status --summarize 2>/dev/null" reached the internal timeout of 20 minutes. Aborting.

The one thing I tried was building with optimization. All of the examples I've found of people using this build with -O0 -g and that is set in that cmake file I linked. I tried removing this so it would build with -O3. That helped it get further but it still had the issue of timing out while running integration tests.

Do you have any ideas on how I could get the integration test to not timeout? Is there some tests I should disable? If I did that though the code coverage number would be artificially too low. The only other idea I have is to do the cmake changes the way I originally suggested. The downside to this is it will be a ton of changes to adjust build options per source file. Let me know if you can think of a simpler way.

[tylerjw]

I tested this working over on moveit_grasps. Here is an example of the sort of report it generates: https://codecov.io/gh/ros-planning/moveit_grasps/branch/p%2Ftylerjw%2Fcodecov

I will try to figure out how to add the unit test to this next.

[tylerjw]

@rhaschke Taking this out of WIP as I think it is ready again for a review. I added to the unit test for it and have verified it works on moveit_grasps. After reading the code and testing what that the moveit library depends on for existing code coverage generation it works better than what I had here so I revised my readme and the travis script to use that method.

[davetcoleman]

lgtm, but need second review

[davetcoleman]

I'm wondering what this would imply for the current effort to get CircleCi+Codecov running for MoveIt 2: moveit/moveit2#44

@ruffsl

Also, having @mikeferguson look at this would be ideal as he made the original package.

[tylerjw]

@rhaschke I believe I've addressed all your feedback on this. Can we submit this and I'll start a new PR if I find I need to make additional changes as I integrate this with moveit?

[rhaschke]

Two more nitpicks. Then this is ready for merging.

[rhaschke]

Before merging this, I would like to see that tests are actually ignored. In moveit/moveit#1719, I see that you are still struggling with this. Is it possible that coverage reports are accumulated, so you don't see if tests are ignored now when they were not ignored in the past?

[tylerjw]

@rhaschke I'll let you know when I get that working. Hopefully very soon.

[mikeferguson]

I just released 0.3.0 with @rhaschke improvements - once that propagates through the build farm and gets sync'd you'll be able to drop this line

[tylerjw]

Which line? The find_package or the include? Can you link me to your changes?

[mikeferguson]

The include -- see mikeferguson/code_coverage#15 for details (again, I just did the release, it's probably 2 weeks before it syncs all the way through the build farm).

[tylerjw]

Ah, cool. Hopefully I'll get this merged quickly and then I can make a small PR to remove this line once your change propagates.

[tylerjw]

I just got it to work! @rhaschke checkout my latest changes. I couldn't figure out a way to exclude all test directories through the codecov.yml however digging through the options of the bash uploader for codecov.io I found an argument that is then passed to a find instruction before it complies the report. With this change any path that contains a directory called test is excluded from the reporting.

Here is that report: https://codecov.io/gh/ros-planning/moveit/tree/da2720be583e547e4c0abee91b820781834547f3

For now only moveit_core is being tested (as that is the only part that is built with the --coverage flag). But one thing I discovered is that some of the tests run in the rest of the repo outside of moveit_core test lines inside moveit_core.

[rhaschke]

We are slowly converging! Great work. Thanks a lot.

[rhaschke]

I'm curious: How does this effect ccache? Isn't this a debugging option of gcc?

[tylerjw]

That idea came from this: https://cppisland.wordpress.com/2018/12/30/ccache-quick-guide/

The basics is that with a Debug build there is a chance the hit rate is low because of paths in the debug symbols and this is one way to fix that. This should have no affect on non-debug builds.

[rhaschke]

Thanks for providing the link. However, their use case is different: They are building the same stuff in different folders, while for us all files reside in the very same folder between build attempts. Hence, there shouldn't be a cache miss due to cwd.
Did you look into the log files generated with export CCACHE_DEBUG=1 between to consecutive (local) builds?

[tylerjw]

@rhaschke The reason I'm adding ccache changes to this PR is because I think debug symbols are the reason I'm getting so bad of cache performance and that is a big part of why the builds are so slow. On the plus side here is the first full (assuming I added the cmake changes to everywhere I should have) coverage report for moveit:

https://codecov.io/gh/PickNikRobotics/moveit/tree/70809b9ac08d5baf6ddeed521622e79ae3e680d4

[tylerjw]

Some other things I want to try to see if they work better to improve ccache:

Increase cache size to 20G (not sure the impact on travis cache storage).
Set CCACHE_BASEDIR=$(pwd) instead of that compiler debug flag

[tylerjw]

See my PR against @rhaschke 's other codecov PR.