Re: [Tarantool-patches] [PATCH 1/2 v2] cmake: add code coverage support

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
LGTM, just a minor nits below.

On 02.08.23, Sergey Bronnikov via Tarantool-patches wrote:
> Hi, Max
> 
> On 8/2/23 11:06, Maxim Kokryashkin via Tarantool-patches wrote:
> > Hi, Sergey!
> > Thanks for the fixes!
> > LGTM, except for a few comments below.
> >
> > Side note: I see that coverage job in CI is red. Why is that
> > happening?
> 
> This happened because from time to time total code coverage number 
> changes a bit.
> 
> It is really annoying, to solve this we need to increase the threshold 
> in Coveralls service.

I see that now this job is green. Was it fixed?

> 
> 
> 
> >
> > On Tue, Aug 01, 2023 at 09:46:08PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> >> From: Sergey Bronnikov <sergeyb@tarantool.org>
> >>
> >> The patch adds building code coverage report using gcovr [1] and gcov.
> >> gcovr is a better version of lcov, see [2]. There were two new CMake
> >> targets added: LuaJIT-coverage proccess *.gcno and *.gcda files with
> > Typo: s/process/processes/
> Fixed.
> >> gcov, builds a detailed HTML report and prints a summary, target
> >> coverage executes LuaJIT-tests and then runs LuaJIT-coverage. Target
> >> LuaJIT-coverage is useful for building code coverage report for a custom
> >> set of regression tests.
> >>
> >> ```
> >> $ cmake -S . -B build -DENABLE_COVERAGE=ON
> >> $ cmake --build build --parallel --target coverage
> >>
> >> <snipped>
> >>
> >> lines: 84.1% (26056 out of 30997)
> >> functions: 88.8% (2055 out of 2314)
> >> branches: 71.5% (14801 out of 20703)
> >> ```
> >>
> >> 1. https://gcovr.com/
> >> 2. https://gcovr.com/en/stable/faq.html#what-is-the-difference-between-lcov-and-gcovr
> >> ---
> >>   CMakeLists.txt                        |  9 ++++++
> >>   cmake/CodeCoverage.cmake              | 45 +++++++++++++++++++++++++++
> >>   test/CMakeLists.txt                   |  7 +++++
> >>   test/tarantool-c-tests/CMakeLists.txt |  6 +++-
> >>   4 files changed, 66 insertions(+), 1 deletion(-)
> >>   create mode 100644 cmake/CodeCoverage.cmake
> >>
> >> diff --git a/CMakeLists.txt b/CMakeLists.txt
> >> index 6ef24bba..fe6582fa 100644
> >> --- a/CMakeLists.txt
> >> +++ b/CMakeLists.txt
> >> @@ -116,6 +116,15 @@ if(LUAJIT_ENABLE_WARNINGS)
> >>     )
> >>   endif()
> >>   

I suggest to add comment here, that the user should run tests _before_
coverage report, or this may be confusing (yes, I'm this user :)):

| $ make LuaJIT-coverage
| Building coverage report
| lines: 0.0% (0 out of 23883)
| functions: 0.0% (0 out of 1765)
| branches: 0.0% (0 out of 17131)
| Built target LuaJIT-coverage

> >> +set(LUAJIT_ENABLE_COVERAGE_DEFAULT OFF)
> >> +option(LUAJIT_ENABLE_COVERAGE
> >> +       "Enable integration with gcovr, a code coverage program"
> >> +       ${LUAJIT_ENABLE_COVERAGE_DEFAULT})
> >> +if (LUAJIT_ENABLE_COVERAGE)
> >> +  AppendFlags(CMAKE_C_FLAGS --coverage)
> >> +  include(CodeCoverage)
> >> +endif(LUAJIT_ENABLE_COVERAGE)
> >> +
> >>   # Auxiliary flags for main targets (libraries, binaries).
> >>   AppendFlags(TARGET_C_FLAGS
> >>     -D_FILE_OFFSET_BITS=64
> >> diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake
> >> new file mode 100644
> >> index 00000000..2be7d129
> >> --- /dev/null
> >> +++ b/cmake/CodeCoverage.cmake
> >> @@ -0,0 +1,45 @@
> >> +find_program(GCOVR gcovr)
> >> +find_program(GCOV gcov)
> >> +
> >> +set(COVERAGE_DIR "${PROJECT_BINARY_DIR}/coverage")
> >> +set(COVERAGE_HTML_REPORT "${COVERAGE_DIR}/luajit.html")
> >> +set(COVERAGE_XML_REPORT "${COVERAGE_DIR}/luajit.xml")
> >> +
> >> +if(NOT GCOVR OR NOT GCOV)
> >> +  add_custom_target(${PROJECT_NAME}-coverage
> >> +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target"

I suggest to split this line into several too.

> >> +  )
> >> +  message(WARNING "Either `gcovr' or `gcov` not found, \
> >> +so ${PROJECT_NAME}-coverage target is dummy")
> > Nit: Something is wrong with alignment here.
> No, it is intentionally. If you add indentation then these whitespaces 
> will be added to a message.

Works just fine with the following diff for me:

===================================================================
diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake
index 2be7d129..83e23d7f 100644
--- a/cmake/CodeCoverage.cmake
+++ b/cmake/CodeCoverage.cmake
@@ -9,8 +9,8 @@ if(NOT GCOVR OR NOT GCOV)
   add_custom_target(${PROJECT_NAME}-coverage
     COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target"
   )
-  message(WARNING "Either `gcovr' or `gcov` not found, \
-so ${PROJECT_NAME}-coverage target is dummy")
+  message(WARNING "Either `gcovr' or `gcov` not found, "
+                  "so ${PROJECT_NAME}-coverage target is dummy")
   return()
 endif()
 
===================================================================

<snipped>

>     # Exclude DynASM files, that contain a low-level VM code for CPUs.
>     --exclude ".*\.dasc"
>     # Exclude buildvm source code, it's the project's infrastructure.
>     --exclude ".*/host/"

Why don't use ${PROJECT_SOURCE_DIR} instead of .* here?

<snipped>

> >>

-- 
Best regards,
Sergey Kaplun