Skip to content

Conversation

@rhaschke
Copy link
Contributor

As we are planning to use this package for MoveIt's Travis checks (moveit/moveit_ci#91), I would like to incorporate some simplifications, fixes, and clarifications before:

  • include(CodeCoverage) shouldn't be required explicitly. Rather these definitions should be included via find_package() already.
  • Defining the option ENABLE_COVERAGE_TESTING in the package's cmake file doesn't make sense. It needs to be defined in the module included by downstream packages.
  • The README should clarify that this package provides two things:
    • It allows the instrumentation of the generated code for coverage reporting - via APPEND_COVERAGE_COMPILER_FLAGS()
    • It allows to generate the html coverage report - via add_code_coverage()

@mikeferguson, would be great, if you could create a new release into Kinetic and Melodic if this has been merged.

- automatically include(CodeCoverage)
- clarify that APPEND_COVERAGE_COMPILER_FLAGS() needs to be called before defining any target
set(COVERAGE_EXCLUDES "*/${PROJECT_NAME}/test*" "*/${PROJECT_NAME}/other_dir_i_dont_care_about*")
add_code_coverage(
NAME ${PROJECT_NAME}_coverage
NAME ${PROJECT_NAME}_coverage_report
Copy link
Owner

Choose a reason for hiding this comment

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

Seems reasonable, after merge I'll update the docs below so the names match

@mikeferguson mikeferguson merged commit f70917f into mikeferguson:master Nov 19, 2019
@mikeferguson
Copy link
Owner

@rhaschke Thanks! All good ideas. 0.3.0 release is going out to kinetic/melodic now

martiniil added a commit to PilzDE/pilz_robots that referenced this pull request Nov 22, 2019
* add missing dependencies of tests
* remove superfluous dependencies of tests
* adapt to changes from mikeferguson/code_coverage#15
martiniil added a commit to PilzDE/pilz_industrial_motion that referenced this pull request Nov 22, 2019
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.

2 participants