Re: [Tarantool-patches] [PATCH] cmake: add code coverage support

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

Hello, Sergey! Thanks for review!


On 7/26/23 18:02, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> It's very interesting to see coverage statistics of the LuaJIT by our
> tests: have much more work to do! :)
>
> Also, it's interesting to compare test suites with each other (done with
> lcov, sorted alphabetically):
> | lua-Harness-tests:
> |   lines......: 54.2% (13550 of 25023 lines)
> |   functions..: 62.4% (1103 of 1769 functions)
> |   branches...: 37.3% (8293 of 22222 branches)
> | LuaJIT-tests:
> |   lines......: 71.3% (17846 of 25023 lines)
> |   functions..: 74.1% (1310 of 1769 functions)
> |   branches...: 54.7% (12155 of 22222 branches)
> | PUC-Rio-Lua-5.1-tests:
> |   lines......: 51.2% (12800 of 25023 lines)
> |   functions..: 57.9% (1025 of 1769 functions)
> |   branches...: 36.0% (7997 of 22222 branches)
> | tarantool-c-tests:
> |   lines......: 33.3% (8210 of 24676 lines)
> |   functions..: 38.6% (672 of 1740 functions)
> |   branches...: 20.4% (4490 of 22014 branches)
> | tarantool-tests:
> |   lines......: 69.6% (17405 of 25023 lines)
> |   functions..: 76.7% (1357 of 1769 functions)
> |   branches...: 49.9% (11090 of 22222 branches)
>
> | Overall:
> |   lines......: 86.0% (21509 of 25023 lines)
> |   functions..: 90.8% (1606 of 1769 functions)
> |   branches...: 66.7% (14812 of 22222 branches)
>
> Please, consider my comments below.
It is interesting, thanks for measuring it!
>
> On 25.07.23, Sergey Bronnikov 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]. CMake target LuaJIT-coverage
> Do we really need any features provided by gcovr that are omitted in
> lcov? It's not a problem for me, but there are many more dependencies
> for gcovr than for lcov. Also, it uses python that isn't such
> available (I mean installed by default) as perl.

gcovr has exactly three dependencies, see [1].

Other dependencies specified for gcovr ebuild are "test" dependencies, 
required for

those who will make patches and run tests for gcovr. I don't get why 
ebuild maintainer

made these dependencies mandatory.

1. 
https://github.com/gcovr/gcovr/blob/27e6b3aaec7c91f54fa8e7531f8d652bf6aeadd4/setup.py#L54


BTW lcov has a bigger number of dependencies than specified

in ebuild (or these dependencies are included in a Perl distribution, 
see [2]:


   - Capture::Tiny
   - DateTime
   - Devel::Cover
   - Digest::MD5
   - File::Spec
   - at least one flavor of JSON module.
     In order of performance/preference:
        - JSON::XS
        - Cpanel::JSON::XS
        - JSON::PP
        - JSON
  - Memory::Process
  - Time::HiRes


2. https://github.com/linux-test-project/lcov


>
> | $ equery depgraph dev-util/gcovr
> | ...
> |  * dependency graph for dev-util/gcovr-6.0
> |  `--  dev-util/gcovr-6.0  [~amd64 keyword]
> |    `--  dev-python/jinja-3.1.2  (dev-python/jinja) amd64
> |    `--  dev-python/lxml-4.9.2  (dev-python/lxml) amd64
> |    `--  dev-python/pygments-2.15.1  (dev-python/pygments) amd64
> |    `--  dev-python/yaxmldiff-0.1.0  (dev-python/yaxmldiff) amd64
> |    `--  dev-lang/python-3.11.4  (dev-lang/python) amd64
> |    `--  dev-python/pytest-timeout-2.1.0  (dev-python/pytest-timeout) amd64
> |    `--  dev-python/pytest-7.3.2  (>=dev-python/pytest-7.3.1) amd64
> |    `--  dev-python/gpep517-13  (>=dev-python/gpep517-13) amd64
> |    `--  dev-python/setuptools-67.8.0-r1  (>=dev-python/setuptools-67.8.0-r1) amd64
> | [ dev-util/gcovr-6.0 stats: packages (10), max depth (1) ]
> | $ equery depgraph dev-util/lcov
> | ...
> |  * dependency graph for dev-util/lcov-1.15
> |  `--  dev-util/lcov-1.15  ~amd64
> |    `--  dev-lang/perl-5.38.0-r1  (dev-lang/perl) ~amd64
> |    `--  dev-perl/JSON-4.100.0  (dev-perl/JSON) ~amd64
> |    `--  dev-perl/PerlIO-gzip-0.200.0-r1  (dev-perl/PerlIO-gzip) amd64
> | [ dev-util/lcov-1.15 stats: packages (4), max depth (1) ]
>
> If we don't really need "decisions" metrics that are provided by default
> by gcovr I suggest to consider to use lcov instead, since I don't see
> any other advantages.
>
>> executes regression tests, proccess *.gcno and *.gcda files with gcov,
>> builds detailed HTML report and prints summary about code coverage.
>>
>> ```
>> $ cmake -S . -B build -DENABLE_COVERAGE=ON
> I suggest to use -DLUAJIT_ENABLE_COVERAGE=ON to be consistent with other
> options.
I don't mind, renamed to LUAJIT_ENABLE_COVERAGE.
>
>> $ cmake --build build --parallel
>> $ cmake --build build --target LuaJIT-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 ++++++
>>   test/CMakeLists.txt                   | 40 +++++++++++++++++++++++++++
>>   test/tarantool-c-tests/CMakeLists.txt |  2 +-
>>   3 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>> index 6ef24bba..8bc63b90 100644
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -116,6 +116,15 @@ if(LUAJIT_ENABLE_WARNINGS)
>>     )
>>   endif()
>>   
>> +set(ENABLE_COVERAGE_DEFAULT OFF)
>> +option(ENABLE_COVERAGE "Enable integration with gcovr, a code coverage program" ${ENABLE_COVERAGE_DEFAULT})
> Minor: Please split the line to the several, to avoid too long lines.

Fixed.


>
>> +if (ENABLE_COVERAGE)
>> +  AppendFlags(CMAKE_C_FLAGS
>> +    -fprofile-arcs
>> +    -ftest-coverage
> Should we use just --coverage?
Sure, updated.
>
> | --coverage
> |            This option is used to compile and link code instrumented
> |            for coverage analysis.  The option is a synonym for
> |            -fprofile-arcs -ftest-coverage (when compiling) and -lgcov
> |            (when linking).
>
> See man gcc(1) for details.
>
>> +  )
>> +endif()
>> +
>>   # Auxiliary flags for main targets (libraries, binaries).
>>   AppendFlags(TARGET_C_FLAGS
>>     -D_FILE_OFFSET_BITS=64
>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>> index 47296a22..9b0f11d8 100644
>> --- a/test/CMakeLists.txt
>> +++ b/test/CMakeLists.txt
>> @@ -59,6 +59,44 @@ add_custom_target(${PROJECT_NAME}-test DEPENDS
>>     tarantool-tests
>>   )
>>   
>> +find_program(GCOVR gcovr)
>> +find_program(GCOV gcov)
>> +set(COVERAGE_HTML_REPORT "luajit-coverage.html")
>> +set(COVERAGE_JSON_REPORT "luajit-coverage.json")
> Should we add ${PROJECT_BUILD_DIR} prefix to be more graceful for
> out-of-source build?
> Also, I see tons of html helper files in the working directory. Is there
> any way to use some <coverage/> directory for all this stuff instead?

Updated:


--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -61,9 +61,10 @@ add_custom_target(${PROJECT_NAME}-test DEPENDS

  find_program(GCOVR gcovr)
  find_program(GCOV gcov)
-set(COVERAGE_HTML_REPORT "luajit-coverage.html")
-set(COVERAGE_JSON_REPORT "luajit-coverage.json")
-if(ENABLE_COVERAGE)
+set(COVERAGE_DIR "${PROJECT_BINARY_DIR}/coverage")
+set(COVERAGE_HTML_REPORT "${COVERAGE_DIR}/luajit.html")
+set(COVERAGE_JSON_REPORT "${COVERAGE_DIR}/luajit.json")


>
>> +if(ENABLE_COVERAGE)
>> +  if(NOT GCOVR OR NOT GCOV)
>> +    add_custom_target(${PROJECT_NAME}-coverage)
>> +    add_custom_command(TARGET ${PROJECT_NAME}-coverage
>> +      COMMENT "Either `gcovr' or `gcov` not found, so ${PROJECT_NAME}-coverage target is dummy"
> Minor: Please split the line to the several, to avoid too long lines.


Done.

>
>> +    )
>> +    return()
> I suppose, we should create dummy target instead.
> | cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF -DENABLE_COVERAGE=OFF && make -j
> | -- [SetVersion] Reading version from VCS: v2.1.0-beta3-331-g4a26e6a1
> | -- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 4
> | CMake Error at test/CMakeLists.txt:118 (add_dependencies):
> |   Cannot add target-level dependencies to non-existent target
> |   "LuaJIT-coverage".
>
>> +  endif()
>> +
>> +  add_custom_target(${PROJECT_NAME}-coverage)
>> +  add_custom_command(TARGET ${PROJECT_NAME}-coverage
>> +    COMMENT "Building coverage report"
>> +    COMMAND
>> +      ${GCOVR}
>> +        # See https://gcovr.com/en/stable/guide/configuration.html
>> +        --root ${PROJECT_SOURCE_DIR}
>> +        --filter ${PROJECT_SOURCE_DIR}/src
> I suppose some files like *.dasc files or buildvm sources should be
> excluded too, since they are not informative.

Done.


>> +        --print-summary
>> +        --output ${COVERAGE_HTML_REPORT}
>> +        --coveralls ${COVERAGE_JSON_REPORT}
> I see also output
>
>> +        --html
>> +        --html-title "LuaJIT Code Coverage Report"
>> +        --html-details
>> +        --sort-percentage
> I can't see an option for decreasing percentage of uncovered lines?
> Is its default value? If it is, then it should be used (as far as we want
> to see uncovered cases first).


Seems it can sort in that order only.

>
>> +        --branches
>> +        --decisions
> Do we need this option? AFAICS, this is mostly about Functional Safety
> applications (road vehicles, etc.).

I would leave it.


>
>> +        --verbose
> I suppose that we should use --verbose option only when VERBOSE=1 env
> var is set.
>
> Also, I'm suprised by lines like this:
> | (DEBUG) Parsing coverage data for file /home/burii/reviews/luajit/gcov/src/lj_tab.h
> | (DEBUG) Starting the decision analysis
> | (DEBUG) Decision Analysis finished!
> | (DEBUG) Finding source file corresponding to a gcov data file
> Is it the part of the --verbose options?

Right, it is enabled only when --verbose is passed. Removed option 
--verbose at all,

I believe we don't need it in normal use.


>
>> +        -j ${CMAKE_BUILD_PARALLEL_LEVEL}
>> +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>> +  )
>> +  message(STATUS "Code coverage HTML report: ${COVERAGE_HTML_REPORT}")
>> +  message(STATUS "Code coverage JSON report: ${COVERAGE_JSON_REPORT}")
>> +endif(ENABLE_COVERAGE)
>> +
>>   if(LUAJIT_USE_TEST)
>>     if(POLICY CMP0037)
>>       if(CMAKE_VERSION VERSION_LESS 3.11)
>> @@ -76,4 +114,6 @@ if(LUAJIT_USE_TEST)
>>       ${PROJECT_NAME}-test
>>       ${PROJECT_NAME}-luacheck
>>     )
>> +
>> +  add_dependencies(${PROJECT_NAME}-coverage ${PROJECT_NAME}-test)
> Is it necessary to rerun all tests to generate this analisys every
> time?

What could be a reasons to regenerate  coverage report without rerunning 
tests?

I can make a target LuaJIT-coverage that will generate report and add 
target coverage that will

run tests and then call LuaJIT-coverage. What do you think?


>
>>   endif()
>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>> index 17255345..8cd76b44 100644
>> --- a/test/tarantool-c-tests/CMakeLists.txt
>> +++ b/test/tarantool-c-tests/CMakeLists.txt
>> @@ -45,7 +45,7 @@ foreach(test_source ${tests})
>>       OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"
>>       RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
>>     )
>> -  target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
>> +  target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY} --coverage)
> Why is it done unconditionally?

Ah, missed it. Thanks!


--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -45,7 +45,11 @@ foreach(test_source ${tests})
      OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"
      RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
    )
-  target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY} --coverage)
+  set(libtest-libs libtest ${LUAJIT_LIBRARY})
+  if (LUAJIT_ENABLE_COVERAGE)
+    set(libtest-libs ${libtest-libs} --coverage)
+  endif (LUAJIT_ENABLE_COVERAGE)
+  target_link_libraries(${exe} ${libtest-libs})
    LIST(APPEND TESTS_COMPILED ${exe})
  endforeach()

>
>>     LIST(APPEND TESTS_COMPILED ${exe})
>>   endforeach()
>>   
>> -- 
>> 2.34.1
>>