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.

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

Adding code coverage report to build #553

Merged
merged 25 commits into from
Feb 13, 2019

Conversation

crdelsey
Copy link
Contributor

@crdelsey crdelsey commented Feb 2, 2019

Basic Info

Adding code coverage generation to the travis build with a report out on Codecov.io.

  • I've split the travis CI build into two:
    • a release build that eventually gets pushed to dockerhub, and
    • a debug build that includes code coverage generation. The results to that build get pushed to codecov.io
  • I added a script to to the tools folder that runs the test suite, so that only one line is needed in the travis.yml file instead of 4 or so.
  • I added a script to the tools folder that generates the code coverage data. If you run it without options it generates an html report you can view locally. If you run it with the option codecovio, it uploads the data to codecov.io which generates a report.
  • I could not use colcon/colcon-lcov-result on account of Header files in install directories are filtered out of the results colcon/colcon-lcov-result#9, but will keep an eye on it for future refactoring.
  • The reports generated on codecov.io are a bit wonky because we don't have a baseline report yet. I think I can't clean up any further issues with codecov.io until we integrate this PR to get baseline data.

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0deebc1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #553   +/-   ##
=========================================
  Coverage          ?   37.88%           
=========================================
  Files             ?      216           
  Lines             ?     9006           
  Branches          ?     3516           
=========================================
  Hits              ?     3412           
  Misses            ?     3384           
  Partials          ?     2210

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 0deebc1...6cf7498. Read the comment docs.

@crdelsey crdelsey mentioned this pull request Feb 4, 2019
12 tasks
@crdelsey crdelsey changed the title [WIP] Adding code coverage report to build Adding code coverage report to build Feb 4, 2019
@crdelsey
Copy link
Contributor Author

crdelsey commented Feb 6, 2019

Fixes #498

@ruffsl
Copy link
Member

ruffsl commented Feb 7, 2019

Is this something we'd could for PRs as well? What is the time overhead like when running those code coverage steps?

@crdelsey
Copy link
Contributor Author

crdelsey commented Feb 8, 2019

Is this something we'd could for PRs as well? What is the time overhead like when running those code coverage steps?

Yes. This will run on PRs. The coverage steps themselves take on the order of 30 seconds. The biggest overhead is that now we build the stack a second time just to collect coverage data,

Copy link
Collaborator

@mkhansenbot mkhansenbot left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for adding this!

name: Report Code Coverage
command: |
export ci_env=`bash <(curl -s https://codecov.io/env)`
. install/setup.sh
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to source the workspace to run codecove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, but I'm not 100% sure. It might be that we have to do it because we don't have all the right exec dependencies listed in our package.xml files.

.circleci/config.yml Outdated Show resolved Hide resolved
@ruffsl
Copy link
Member

ruffsl commented Feb 8, 2019

Yes. This will run on PRs. The coverage steps themselves take on the order of 30 seconds. The biggest overhead is that now we build the stack a second time just to collect coverage data,

It looks like the CircleCI was missing the run steps to upload the codecove files for PRs, so I added 38bb5fa to try and include that. Though I'm not sure I understand the environment setup codecov needs, so please check that I've invoked it correctly. Travis RP job's codcov upload might overwrite CircleCI upload though, so we may want to pick just one for PRs.

.circleci/config.yml Outdated Show resolved Hide resolved
@@ -186,6 +199,7 @@ executors:
working_directory: /opt/nav2_ws
environment:
CMAKE_BUILD_TYPE: "Release"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a COVERAGE_ENABLED build, it'll need to be CMAKE_BUILD_TYPE=Debug to produce a useful report. With the release optimizations, the coverage data will probably be unusable.

We'll also need to set a token in the CI environment that the upload script uses. See https://codecov.io/gh/ros-planning/navigation2/settings
I believe you have access.

However, before we set that for circle CI, I think we should only upload from either Travis or Circle. Uploading twice for the same PR might be an issue. I was going to see how much of our 1000 minutes we were using before adding more to the CircleCI build. Do you know how to check the amount of time we've used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind. I just noticed you said the exact same thing.

Copy link
Member

@ruffsl ruffsl Feb 9, 2019

Choose a reason for hiding this comment

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

For a COVERAGE_ENABLED build, it'll need to be CMAKE_BUILD_TYPE=Debug to produce a useful report. With the release optimizations, the coverage data will probably be unusable.

Should we just default to Debug builds for PRs, using a nightly cron on master to build/test both. The cron could have a forked workflow:

  • testing with Debug (and thus codecove for master branch)
  • testing Release (to make sure at least master is periodically Release tested)

I was going to see how much of our 1000 minutes we were using before adding more to the CircleCI build. Do you know how to check the amount of time we've used?

I can't see this, but org admins should be able to view the stats here:
https://circleci.com/gh/organizations/ros-planning/settings#usage

Although, open source projects (jobs for public repos I presume) shouldn't be affected.

https://discuss.circleci.com/t/has-the-build-minutes-per-month-in-the-free-tier-been-reduced/26399/3

However, before we set that for circle CI, I think we should only upload from either Travis or Circle.

I could comment out the codecov line in Travis to just test out Circleci. Once we merge, circleci config used for PRs would be updated, so we could then profile the added time for codecov steps.

@ruffsl
Copy link
Member

ruffsl commented Feb 9, 2019

I changed the workflow to trigger parallel builds for Release and Debug. I'm not sure if we need to test release on PRs as well, but currently only the debug job includes the command to upload to codecov. We can rework the trigger filter to be whatever combination we'd need, like only testing both on master branch, ect.

@crdelsey
Copy link
Contributor Author

@orduno @mhpanah @ruffsl Looks like this is now passing all the CI it can at the moment. It needs one more approver before it can go in. If one of you can review and approve, I'd appreciate it.

Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM

@crdelsey crdelsey merged commit 357e303 into ros-navigation:master Feb 13, 2019
crdelsey pushed a commit to crdelsey/navigation2 that referenced this pull request May 20, 2019
* Script to generate coverage report. Should be run after test.

* Add helper script for test suite

* Modified travis to use runtestsuite.bash script

* Add coverage to .travis.yml file.

* Add coverage badge to main page

* Run the code coverage report in the docker container.

* Add lcov dependency.

* Trying to add lcov tool to build.

* Add coverage args to build script.

* Fixing passing code coverage args.

* Fixing setting env variable.

* Fixing docker file coverage_enabled flag.

* Fixing coverage report script.

* Pass ci env through to docker

* Fixing the filter rules for this workspace.

* Removing unneeded dependency install line.

* Add comments explaining what lcov lines do.

* Rebasing on top of circle CI and common Cmake functions changes.

* Fixing test suite script name.

* Add codecov to CircleCI jobs

* Add conditional to skip codecov if enable is false

* Remove unnecessary export for codecov

* Update workflow for paralel release and debug jobs

* Forgot to append coverage step to job

* Comment out travis codecove
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.

3 participants