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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the fixes!
LGTM, except for a single comment below.

On Mon, Aug 07, 2023 at 04:39:27PM +0300, Sergey Bronnikov wrote:
> Hello, Sergey!
> 
> 
> On 8/6/23 14:35, Sergey Kaplun wrote:
> > 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?
> Actually no. I'll ask someone who has access to settings to increase
> threshold.
> > 
> > > 
> > > 
> > > > 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
> 
> The difference for LuaJIT-coverage and coverage targets is described in
> commit message.
> 
> Comment is already there:
> 
> >   add_custom_command(TARGET ${PROJECT_NAME}-coverage
> >    COMMENT "Building coverage report"
> 
> 
> > 
> > > > > +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)

I believe it would be better to do that in the `test/CMakeLists.txt`
instead of the main one, since coverage is semantically relevant to tests.
Feel free to ignore.

> > > > > +
> > > > >    # 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.
> 
> splitted
> 
> 
> > 
> > > > > +  )
> > > > > +  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()
> > ===================================================================
> 
> Applied, thanks!
> 
> 
> > 
> > <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?
> 
> 
> It is not needed here. gcovr searches *.gcda/*.gcno files in
> PROJECT_BINARY_DIRECTORY
> 
> and additionally all paths excluded except PROJECT_SOURCE_DIR/src. So
> absolute path is excessive in regexes specified in --exclude options.
> 
> > 
> > <snipped>
> 
> 
> 
> >