Test coverage #1073

emye · 2019-12-10T01:59:12Z

Implements creation of local coverage reports using gcovr as stated in #1068

Example usage:

cmake -DBUILD_WITH_COVERAGE=yes ../
make all
make tests
make coverage

It will create a directory called coverage_output inside the CMAKE_BINARY_DIR directory, storing the html output from gcovr.

codecov · 2019-12-10T02:26:41Z

Codecov Report

Merging #1073 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1073   +/-   ##
======================================
  Coverage    71.4%   71.4%           
======================================
  Files         148     148           
  Lines       17439   17439           
======================================
  Hits        12453   12453           
  Misses       4986    4986

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 a55451b...969e6db. Read the comment docs.

piponazo

Thanks for the contribution!

After some trial and error I could finally obtain the desired coverage reports 🎊 . It is important to note that the gcov and gcc versions must match. I had some issues because of that, since I have many gcc versions installed in my system and I the gcov version was not matching with the gcc one.

I left some suggestions to improve a little bit the code. Once these comments are addressed I will be happy to merge the changes.

piponazo · 2019-12-10T07:21:17Z

CMakeLists.txt

@@ -8,6 +8,9 @@ project(exiv2
 include(cmake/mainSetup.cmake  REQUIRED)
 include(CTest)

+list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake)
+include (gcovr)


[minor] Although this is correct, we are trying in this project to not modify the CMAKE_MODULE_PATH internally so that we can clearly differentiate between the CMake provided modules and the ones provided by us.

Please remove line 11 and change line 12 to:

include(cmake/gcovr.cmake REQUIRED)

piponazo · 2019-12-10T07:22:49Z

cmake/gcovr.cmake

@@ -0,0 +1,16 @@
+if(USE_GCOV)


We should create a option in the main CMakeLists.txt for this variable. Ideally we should reuse the existing one for remote coverage BUILD_WITH_COVERAGE.

Note that the line include(cmake/gcovr.cmake REQUIRED) should appear after that option.

@piponazo, should we continue using the new gcovr.cmake file, or should we append the code to BUILD_WITH_COVERAGE within the compilerFlags.cmake file?

I do not have a strong opinion about that. Either you add it in compilerFlags.cmake well separated from the "generic" compiler flags or you keep it in that new file. Up to you 😉

piponazo · 2019-12-10T07:24:13Z

cmake/gcovr.cmake

@@ -0,0 +1,16 @@
+if(USE_GCOV)
+
+    SET(GCOVR gcovr)


Here we are assuming the existence of gcovr. Please, use find_program to check whether it exists:

https://cmake.org/cmake/help/v3.5/command/find_program.html

piponazo · 2019-12-10T07:26:59Z

cmake/gcovr.cmake

+
+    SET(GCC_COVERAGE_COMPILE_FLAGS "-fprofile-arcs -ftest-coverage")
+    SET(GCC_COVERAGE_LINK_FLAGS "-lgcov")
+    SET( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${GCC_COVERAGE_COMPILE_FLAGS}" )


We are moving away from using CMAKE_CXX_FLAGS variables which is considered a bad practice in CMake. Instead of that, we should be using add_compile_options and add_link_options. However, the second one is only available from CMake 3.13 and therefore we keep using CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS instead. You can take a look to the file cmake/compilerFlags.cmake to see some examples.

cmake/gcovr.cmake

piponazo

There is only a missing piece (see the previous comment). I would appreciate if you add the additional changes, but I would not block the PR just because of that.

piponazo · 2019-12-10T18:54:55Z

cmake/gcovr.cmake

-    SET( CMAKE_EXE_LINKER_FLAGS "${CMAKE_LINKER_FLAGS} ${GCC_COVERAGE_LINK_FLAGS}" )
+if(BUILD_WITH_COVERAGE)
+
+    find_program (GCOVR gcovr)


Thanks for the changes.

The only missing piece would be to add a check after this line, something like:

if(NOT GCOVR) message(FATAL_ERROR "gcovr not found") endif()

Just added the change. Thank you very much for the help!

@piponazo , there are some failures in the CI tests due to the lack of gcovr. If a user doesn't have it, should we just not apply the target?

@piponazo , there are some failures in the CI tests due to the lack of gcovr. If a user doesn't have it, should we just not apply the target?

I agree with that suggestion: gcovr is not essential for obtaining the test coverage data on the CI.

@piponazo @D4N We've changed the if statement to if(GCOVR), and put the flags inside the if statement. That should solve the CI problem.

Fine for me as it is! Thank you a lot for the contribution 😄

Thank you! We greatly enjoyed this experience!

Pull request has been modified.

D4N · 2019-12-10T23:06:59Z

This looks very nice, thanks @emye and @piponazo!

D4N · 2019-12-11T05:38:43Z

Lgtm to me now! @piponazo what do you think?

* Compiles and runs * Created gcovr reports in separate directory * Improved style and degree of coverage * Added error check for existence of gcovr * CI test fixes

emye added 2 commits December 8, 2019 19:06

Compiles and runs

d60e6f4

Created gcovr reports in separate directory

cef533d

piponazo requested changes Dec 10, 2019

View reviewed changes

Improved style and degree of coverage

48065d6

piponazo previously approved these changes Dec 10, 2019

View reviewed changes

Added error check for existence of gcovr

5cf70df

CI test fixes

969e6db

piponazo approved these changes Dec 11, 2019

View reviewed changes

piponazo merged commit 24d6feb into Exiv2:master Dec 11, 2019

1div0 pushed a commit to 1div0/exiv2 that referenced this pull request Jun 7, 2020

Test coverage (Exiv2#1073)

6ed6374

* Compiles and runs * Created gcovr reports in separate directory * Improved style and degree of coverage * Added error check for existence of gcovr * CI test fixes

piponazo mentioned this pull request Apr 14, 2021

CMake: add targets to generate local coverage reports #1068

Closed

piponazo mentioned this pull request May 13, 2021

CMake target for creating local coverage reports #1640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test coverage #1073

Test coverage #1073

emye commented Dec 10, 2019 •

edited

Loading

codecov bot commented Dec 10, 2019 •

edited

Loading

piponazo left a comment

piponazo Dec 10, 2019

piponazo Dec 10, 2019

piponazo Dec 10, 2019

emye Dec 10, 2019

piponazo Dec 10, 2019

piponazo Dec 10, 2019

piponazo Dec 10, 2019

piponazo left a comment

piponazo Dec 10, 2019

emye Dec 10, 2019

emye Dec 10, 2019

D4N Dec 10, 2019

emye Dec 11, 2019 •

edited

Loading

piponazo Dec 11, 2019

emye Dec 11, 2019

D4N commented Dec 10, 2019

D4N commented Dec 11, 2019

Test coverage #1073

Test coverage #1073

Conversation

emye commented Dec 10, 2019 • edited Loading

codecov bot commented Dec 10, 2019 • edited Loading

Codecov Report

piponazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emye Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D4N commented Dec 10, 2019

D4N commented Dec 11, 2019

emye commented Dec 10, 2019 •

edited

Loading

codecov bot commented Dec 10, 2019 •

edited

Loading

emye Dec 11, 2019 •

edited

Loading