<HTML><BODY><div>Hi!</div><div>Just want to add a bit to the Perl vs Python topic. Python comes</div><div>pre-installed on most of the Linux distributions, including RHEL,</div><div>CentOS, Fedora, Debian, Ubuntu and other less popular. It is</div><div>also true for the modern OSX versions. So, unless you really</div><div>want to run coverage on something like FreeBSD, then there</div><div>is virtually no difference whatsoever.</div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16904473871818624348_BODY">Hello, Sergey! Thanks for review!<br><br><br>On 7/26/23 18:02, Sergey Kaplun wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> It's very interesting to see coverage statistics of the LuaJIT by our<br>> tests: have much more work to do! :)<br>><br>> Also, it's interesting to compare test suites with each other (done with<br>> lcov, sorted alphabetically):<br>> | lua-Harness-tests:<br>> | lines......: 54.2% (13550 of 25023 lines)<br>> | functions..: 62.4% (1103 of 1769 functions)<br>> | branches...: 37.3% (8293 of 22222 branches)<br>> | LuaJIT-tests:<br>> | lines......: 71.3% (17846 of 25023 lines)<br>> | functions..: 74.1% (1310 of 1769 functions)<br>> | branches...: 54.7% (12155 of 22222 branches)<br>> | PUC-Rio-Lua-5.1-tests:<br>> | lines......: 51.2% (12800 of 25023 lines)<br>> | functions..: 57.9% (1025 of 1769 functions)<br>> | branches...: 36.0% (7997 of 22222 branches)<br>> | tarantool-c-tests:<br>> | lines......: 33.3% (8210 of 24676 lines)<br>> | functions..: 38.6% (672 of 1740 functions)<br>> | branches...: 20.4% (4490 of 22014 branches)<br>> | tarantool-tests:<br>> | lines......: 69.6% (17405 of 25023 lines)<br>> | functions..: 76.7% (1357 of 1769 functions)<br>> | branches...: 49.9% (11090 of 22222 branches)<br>><br>> | Overall:<br>> | lines......: 86.0% (21509 of 25023 lines)<br>> | functions..: 90.8% (1606 of 1769 functions)<br>> | branches...: 66.7% (14812 of 22222 branches)<br>><br>> Please, consider my comments below.<br>It is interesting, thanks for measuring it!<br>><br>> On 25.07.23, Sergey Bronnikov wrote:<br>>> From: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br>>><br>>> The patch adds building code coverage report using gcovr [1] and gcov.<br>>> gcovr is a better version of lcov, see [2]. CMake target LuaJIT-coverage<br>> Do we really need any features provided by gcovr that are omitted in<br>> lcov? It's not a problem for me, but there are many more dependencies<br>> for gcovr than for lcov. Also, it uses python that isn't such<br>> available (I mean installed by default) as perl.<br><br>gcovr has exactly three dependencies, see [1].<br><br>Other dependencies specified for gcovr ebuild are "test" dependencies,<br>required for<br><br>those who will make patches and run tests for gcovr. I don't get why<br>ebuild maintainer<br><br>made these dependencies mandatory.<br><br>1.<br><a href="https://github.com/gcovr/gcovr/blob/27e6b3aaec7c91f54fa8e7531f8d652bf6aeadd4/setup.py#L54" target="_blank">https://github.com/gcovr/gcovr/blob/27e6b3aaec7c91f54fa8e7531f8d652bf6aeadd4/setup.py#L54</a><br><br><br>BTW lcov has a bigger number of dependencies than specified<br><br>in ebuild (or these dependencies are included in a Perl distribution,<br>see [2]:<br><br><br> - Capture::Tiny<br> - DateTime<br> - Devel::Cover<br> - Digest::MD5<br> - File::Spec<br> - at least one flavor of JSON module.<br> In order of performance/preference:<br> - JSON::XS<br> - Cpanel::JSON::XS<br> - JSON::PP<br> - JSON<br> - Memory::Process<br> - Time::HiRes<br><br><br>2. <a href="https://github.com/linux-test-project/lcov" target="_blank">https://github.com/linux-test-project/lcov</a><br><br><br>><br>> | $ equery depgraph dev-util/gcovr<br>> | ...<br>> | * dependency graph for dev-util/gcovr-6.0<br>> | `-- dev-util/gcovr-6.0 [~amd64 keyword]<br>> | `-- dev-python/jinja-3.1.2 (dev-python/jinja) amd64<br>> | `-- dev-python/lxml-4.9.2 (dev-python/lxml) amd64<br>> | `-- dev-python/pygments-2.15.1 (dev-python/pygments) amd64<br>> | `-- dev-python/yaxmldiff-0.1.0 (dev-python/yaxmldiff) amd64<br>> | `-- dev-lang/python-3.11.4 (dev-lang/python) amd64<br>> | `-- dev-python/pytest-timeout-2.1.0 (dev-python/pytest-timeout) amd64<br>> | `-- dev-python/pytest-7.3.2 (>=dev-python/pytest-7.3.1) amd64<br>> | `-- dev-python/gpep517-13 (>=dev-python/gpep517-13) amd64<br>> | `-- dev-python/setuptools-67.8.0-r1 (>=dev-python/setuptools-67.8.0-r1) amd64<br>> | [ dev-util/gcovr-6.0 stats: packages (10), max depth (1) ]<br>> | $ equery depgraph dev-util/lcov<br>> | ...<br>> | * dependency graph for dev-util/lcov-1.15<br>> | `-- dev-util/lcov-1.15 ~amd64<br>> | `-- dev-lang/perl-5.38.0-r1 (dev-lang/perl) ~amd64<br>> | `-- dev-perl/JSON-4.100.0 (dev-perl/JSON) ~amd64<br>> | `-- dev-perl/PerlIO-gzip-0.200.0-r1 (dev-perl/PerlIO-gzip) amd64<br>> | [ dev-util/lcov-1.15 stats: packages (4), max depth (1) ]<br>><br>> If we don't really need "decisions" metrics that are provided by default<br>> by gcovr I suggest to consider to use lcov instead, since I don't see<br>> any other advantages.<br>><br>>> executes regression tests, proccess *.gcno and *.gcda files with gcov,<br>>> builds detailed HTML report and prints summary about code coverage.<br>>><br>>> ```<br>>> $ cmake -S . -B build -DENABLE_COVERAGE=ON<br>> I suggest to use -DLUAJIT_ENABLE_COVERAGE=ON to be consistent with other<br>> options.<br>I don't mind, renamed to LUAJIT_ENABLE_COVERAGE.<br>><br>>> $ cmake --build build --parallel<br>>> $ cmake --build build --target LuaJIT-coverage<br>>><br>>> <snipped><br>>><br>>> lines: 84.1% (26056 out of 30997)<br>>> functions: 88.8% (2055 out of 2314)<br>>> branches: 71.5% (14801 out of 20703)<br>>> ```<br>>><br>>> 1. <a href="https://gcovr.com/" target="_blank">https://gcovr.com/</a><br>>> 2. <a href="https://gcovr.com/en/stable/faq.html#what-is-the-difference-between-lcov-and-gcovr" target="_blank">https://gcovr.com/en/stable/faq.html#what-is-the-difference-between-lcov-and-gcovr</a><br>>> ---<br>>> CMakeLists.txt | 9 ++++++<br>>> test/CMakeLists.txt | 40 +++++++++++++++++++++++++++<br>>> test/tarantool-c-tests/CMakeLists.txt | 2 +-<br>>> 3 files changed, 50 insertions(+), 1 deletion(-)<br>>><br>>> diff --git a/CMakeLists.txt b/CMakeLists.txt<br>>> index 6ef24bba..8bc63b90 100644<br>>> --- a/CMakeLists.txt<br>>> +++ b/CMakeLists.txt<br>>> @@ -116,6 +116,15 @@ if(LUAJIT_ENABLE_WARNINGS)<br>>> )<br>>> endif()<br>>><br>>> +set(ENABLE_COVERAGE_DEFAULT OFF)<br>>> +option(ENABLE_COVERAGE "Enable integration with gcovr, a code coverage program" ${ENABLE_COVERAGE_DEFAULT})<br>> Minor: Please split the line to the several, to avoid too long lines.<br><br>Fixed.<br><br><br>><br>>> +if (ENABLE_COVERAGE)<br>>> + AppendFlags(CMAKE_C_FLAGS<br>>> + -fprofile-arcs<br>>> + -ftest-coverage<br>> Should we use just --coverage?<br>Sure, updated.<br>><br>> | --coverage<br>> | This option is used to compile and link code instrumented<br>> | for coverage analysis. The option is a synonym for<br>> | -fprofile-arcs -ftest-coverage (when compiling) and -lgcov<br>> | (when linking).<br>><br>> See man gcc(1) for details.<br>><br>>> + )<br>>> +endif()<br>>> +<br>>> # Auxiliary flags for main targets (libraries, binaries).<br>>> AppendFlags(TARGET_C_FLAGS<br>>> -D_FILE_OFFSET_BITS=64<br>>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt<br>>> index 47296a22..9b0f11d8 100644<br>>> --- a/test/CMakeLists.txt<br>>> +++ b/test/CMakeLists.txt<br>>> @@ -59,6 +59,44 @@ add_custom_target(${PROJECT_NAME}-test DEPENDS<br>>> tarantool-tests<br>>> )<br>>><br>>> +find_program(GCOVR gcovr)<br>>> +find_program(GCOV gcov)<br>>> +set(COVERAGE_HTML_REPORT "luajit-coverage.html")<br>>> +set(COVERAGE_JSON_REPORT "luajit-coverage.json")<br>> Should we add ${PROJECT_BUILD_DIR} prefix to be more graceful for<br>> out-of-source build?<br>> Also, I see tons of html helper files in the working directory. Is there<br>> any way to use some <coverage/> directory for all this stuff instead?<br><br>Updated:<br><br><br>--- a/test/CMakeLists.txt<br>+++ b/test/CMakeLists.txt<br>@@ -61,9 +61,10 @@ add_custom_target(${PROJECT_NAME}-test DEPENDS<br><br> find_program(GCOVR gcovr)<br> find_program(GCOV gcov)<br>-set(COVERAGE_HTML_REPORT "luajit-coverage.html")<br>-set(COVERAGE_JSON_REPORT "luajit-coverage.json")<br>-if(ENABLE_COVERAGE)<br>+set(COVERAGE_DIR "${PROJECT_BINARY_DIR}/coverage")<br>+set(COVERAGE_HTML_REPORT "${COVERAGE_DIR}/luajit.html")<br>+set(COVERAGE_JSON_REPORT "${COVERAGE_DIR}/luajit.json")<br><br><br>><br>>> +if(ENABLE_COVERAGE)<br>>> + if(NOT GCOVR OR NOT GCOV)<br>>> + add_custom_target(${PROJECT_NAME}-coverage)<br>>> + add_custom_command(TARGET ${PROJECT_NAME}-coverage<br>>> + COMMENT "Either `gcovr' or `gcov` not found, so ${PROJECT_NAME}-coverage target is dummy"<br>> Minor: Please split the line to the several, to avoid too long lines.<br><br><br>Done.<br><br>><br>>> + )<br>>> + return()<br>> I suppose, we should create dummy target instead.<br>> | cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF -DENABLE_COVERAGE=OFF && make -j<br>> | -- [SetVersion] Reading version from VCS: v2.1.0-beta3-331-g4a26e6a1<br>> | -- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 4<br>> | CMake Error at test/CMakeLists.txt:118 (add_dependencies):<br>> | Cannot add target-level dependencies to non-existent target<br>> | "LuaJIT-coverage".<br>><br>>> + endif()<br>>> +<br>>> + add_custom_target(${PROJECT_NAME}-coverage)<br>>> + add_custom_command(TARGET ${PROJECT_NAME}-coverage<br>>> + COMMENT "Building coverage report"<br>>> + COMMAND<br>>> + ${GCOVR}<br>>> + # See <a href="https://gcovr.com/en/stable/guide/configuration.html" target="_blank">https://gcovr.com/en/stable/guide/configuration.html</a><br>>> + --root ${PROJECT_SOURCE_DIR}<br>>> + --filter ${PROJECT_SOURCE_DIR}/src<br>> I suppose some files like *.dasc files or buildvm sources should be<br>> excluded too, since they are not informative.<br><br>Done.<br><br><br>>> + --print-summary<br>>> + --output ${COVERAGE_HTML_REPORT}<br>>> + --coveralls ${COVERAGE_JSON_REPORT}<br>> I see also output<br>><br>>> + --html<br>>> + --html-title "LuaJIT Code Coverage Report"<br>>> + --html-details<br>>> + --sort-percentage<br>> I can't see an option for decreasing percentage of uncovered lines?<br>> Is its default value? If it is, then it should be used (as far as we want<br>> to see uncovered cases first).<br><br><br>Seems it can sort in that order only.<br><br>><br>>> + --branches<br>>> + --decisions<br>> Do we need this option? AFAICS, this is mostly about Functional Safety<br>> applications (road vehicles, etc.).<br><br>I would leave it.<br><br><br>><br>>> + --verbose<br>> I suppose that we should use --verbose option only when VERBOSE=1 env<br>> var is set.<br>><br>> Also, I'm suprised by lines like this:<br>> | (DEBUG) Parsing coverage data for file /home/burii/reviews/luajit/gcov/src/lj_tab.h<br>> | (DEBUG) Starting the decision analysis<br>> | (DEBUG) Decision Analysis finished!<br>> | (DEBUG) Finding source file corresponding to a gcov data file<br>> Is it the part of the --verbose options?<br><br>Right, it is enabled only when --verbose is passed. Removed option<br>--verbose at all,<br><br>I believe we don't need it in normal use.<br><br><br>><br>>> + -j ${CMAKE_BUILD_PARALLEL_LEVEL}<br>>> + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}<br>>> + )<br>>> + message(STATUS "Code coverage HTML report: ${COVERAGE_HTML_REPORT}")<br>>> + message(STATUS "Code coverage JSON report: ${COVERAGE_JSON_REPORT}")<br>>> +endif(ENABLE_COVERAGE)<br>>> +<br>>> if(LUAJIT_USE_TEST)<br>>> if(POLICY CMP0037)<br>>> if(CMAKE_VERSION VERSION_LESS 3.11)<br>>> @@ -76,4 +114,6 @@ if(LUAJIT_USE_TEST)<br>>> ${PROJECT_NAME}-test<br>>> ${PROJECT_NAME}-luacheck<br>>> )<br>>> +<br>>> + add_dependencies(${PROJECT_NAME}-coverage ${PROJECT_NAME}-test)<br>> Is it necessary to rerun all tests to generate this analisys every<br>> time?<br><br>What could be a reasons to regenerate coverage report without rerunning<br>tests?<br><br>I can make a target LuaJIT-coverage that will generate report and add<br>target coverage that will<br><br>run tests and then call LuaJIT-coverage. What do you think?<br><br><br>><br>>> endif()<br>>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt<br>>> index 17255345..8cd76b44 100644<br>>> --- a/test/tarantool-c-tests/CMakeLists.txt<br>>> +++ b/test/tarantool-c-tests/CMakeLists.txt<br>>> @@ -45,7 +45,7 @@ foreach(test_source ${tests})<br>>> OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"<br>>> RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"<br>>> )<br>>> - target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})<br>>> + target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY} --coverage)<br>> Why is it done unconditionally?<br><br>Ah, missed it. Thanks!<br><br><br>--- a/test/tarantool-c-tests/CMakeLists.txt<br>+++ b/test/tarantool-c-tests/CMakeLists.txt<br>@@ -45,7 +45,11 @@ foreach(test_source ${tests})<br> OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"<br> RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"<br> )<br>- target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY} --coverage)<br>+ set(libtest-libs libtest ${LUAJIT_LIBRARY})<br>+ if (LUAJIT_ENABLE_COVERAGE)<br>+ set(libtest-libs ${libtest-libs} --coverage)<br>+ endif (LUAJIT_ENABLE_COVERAGE)<br>+ target_link_libraries(${exe} ${libtest-libs})<br> LIST(APPEND TESTS_COMPILED ${exe})<br> endforeach()<br><br>><br>>> LIST(APPEND TESTS_COMPILED ${exe})<br>>> endforeach()<br>>><br>>> --<br>>> 2.34.1<br>>></div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>