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 ADF2A54F455; Wed, 26 Jul 2023 18:06:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ADF2A54F455 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1690384002; bh=zBNIMUtgcUEQ+m2SPFN71eDtdcDa/+9IT5rCTfGnu44=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=BPWYYYszCdlsKw/TJbO/kCvVScBe1FMiXfKrooD4PuD+uWIky8W+SqGaOFeS2NNHZ cpLASGEy+8Y5u4PAtben7n/H13d9ru0pBoaHtu2Vo2qsG3twBF1n7c2ry7LGEPlzfL ta0FQ5pTAe9Vy2UhARkzemTQuBnvlUv3ASrtDrrs= Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [95.163.41.93]) (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 8EE6E54AE1A for ; Wed, 26 Jul 2023 18:06:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8EE6E54AE1A Received: by smtp55.i.mail.ru with esmtpa (envelope-from ) id 1qOg64-001uuO-KQ; Wed, 26 Jul 2023 18:06:41 +0300 Date: Wed, 26 Jul 2023 18:02:13 +0300 To: Sergey Bronnikov Message-ID: References: <4a26e6a1f06191178c385a1df62d61763a5743e3.1690293120.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4a26e6a1f06191178c385a1df62d61763a5743e3.1690293120.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F4F4431F26286E46904DDE2160FD6350E92E04680500F5C4182A05F5380850401A288BB319F229E20A642E4435891C78BAB315C4C8C9B57366E82D1FAC67D070 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76AB1B6FB25ACEDC9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063790C8302959DAAF098638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87DE97FED6E56486FA0FED736E15326B3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B3EA24FE487C076E5A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC42D492BDE3C795E73AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F79006378BD6BC68178B1FFFD81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F79006371BCC3D0A94B9E03222CA9DD8327EE4930A3850AC1BE2E735B17145F0B7815491C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A51D01C93BA09DF0D441F163D21120A6A072F08D70CD5300DCF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF63894FB1499728DAACE2BC909A0485BF54406A70A97C901E39CBABEE27331086D10329F4AAC8FAE2D8A5E5B53F5BD04C97491073FE1B7ECC2A09AB83BFFF9ACA74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXeYlEwgoYE/VjXcrFH9pAw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7691DDF8680DBB8FFB20A642E4435891C7879D0F6AC9C0BD8DBDEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. 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. | $ 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. > $ 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. > +if (ENABLE_COVERAGE) > + AppendFlags(CMAKE_C_FLAGS > + -fprofile-arcs > + -ftest-coverage Should we use just --coverage? | --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? > +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. > + ) > + 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. > + --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). > + --branches > + --decisions Do we need this option? AFAICS, this is mostly about Functional Safety applications (road vehicles, etc.). > + --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? > + -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? > 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? > LIST(APPEND TESTS_COMPILED ${exe}) > endforeach() > > -- > 2.34.1 > -- Best regards, Sergey Kaplun