From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 72B7854D505; Thu, 27 Jul 2023 11:42:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 72B7854D505 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1690447373; bh=oHtxZzbRQoClShzGdIGcIldHv3dY+7R5P8Abntfa2UA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=FLtnW0D8zNsV+r3i4KpRrInfG6+qaaQpdBS5G+7rOOh00HbMp2ryD6HocX2ACgHHk wAC0H9+XnUKf4lCDRVO2mLzyopqxjqSdsF/Ips9j7MWWtK2MuSUpZJJXSAw//obhkb YJFkJTRgDTmcQv4bpyVEbZqzHDq4Lgxa/LGQmdtU= Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [95.163.41.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CA2954D9C03 for ; Thu, 27 Jul 2023 11:42:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CA2954D9C03 Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1qOwaA-002x5Q-Od; Thu, 27 Jul 2023 11:42:51 +0300 Message-ID: <60fc85bf-2ac0-fa2c-9e5c-de9b3b71a726@tarantool.org> Date: Thu, 27 Jul 2023 11:42:50 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com References: <4a26e6a1f06191178c385a1df62d61763a5743e3.1690293120.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F4F4431F26286E46B6C81591349CF48065A036C6B0D898E2182A05F538085040FCABDDA627953DE470898A4FC01377A583C5A4899091C97EDD6036A21E0D445F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71107D7B19CDFFE90EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378997215BCAA11D778638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D880E3F4C2130473952063E50D73C30C2C117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520F8AB6B2BE2218126117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF048DACD9C49003B6BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B6AC040D1D648FE0E476E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C22494854413538E1713F75ECD9A6C639B01B78DA827A17800CE7CDC9B9C0B24BED77731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5B9AB0179EA6B1A3B0848720123F376AD0CEEB0F610DD0A96F87CCE6106E1FC07E67D4AC08A07B9B0CF7CD7A0D5AA5F25CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF6590AAC4EEB18508B5DD4694C8305019554BAE95EAD382E92136197AC2EBC7E9FD9AF20B71F72F3A2D8A5E5B53F5BD0456208C4DE6EBC821F79F21548F8C6590A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj5ZCMWLPPSIwmIdyXZiWGeA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769C3FB74E3BFA4FEE970898A4FC01377A5C9F871340C5A6C40EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] cmake: add code coverage support X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 >> >> 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 >> >> >> >> 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 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 >>