[Tarantool-patches] [PATCH] cmake: add code coverage support
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 27 11:42:50 MSK 2023
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 at 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
>>
More information about the Tarantool-patches
mailing list