Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: Sergey Bronnikov <estetus@gmail.com>,
	max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/2 v2] cmake: add code coverage support
Date: Mon, 7 Aug 2023 16:39:27 +0300	[thread overview]
Message-ID: <71807b71-5445-1ce9-f073-a708f87eb29b@tarantool.org> (raw)
In-Reply-To: <ZM-FZ83YIj26Hpn3@root>

Hello, Sergey!


On 8/6/23 14:35, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, just a minor nits below.
>
> On 02.08.23, Sergey Bronnikov via Tarantool-patches wrote:
>> Hi, Max
>>
>> On 8/2/23 11:06, Maxim Kokryashkin via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the fixes!
>>> LGTM, except for a few comments below.
>>>
>>> Side note: I see that coverage job in CI is red. Why is that
>>> happening?
>> This happened because from time to time total code coverage number
>> changes a bit.
>>
>> It is really annoying, to solve this we need to increase the threshold
>> in Coveralls service.
> I see that now this job is green. Was it fixed?
Actually no. I'll ask someone who has access to settings to increase 
threshold.
>
>>
>>
>>> On Tue, Aug 01, 2023 at 09:46:08PM +0300, Sergey Bronnikov via Tarantool-patches 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]. There were two new CMake
>>>> targets added: LuaJIT-coverage proccess *.gcno and *.gcda files with
>>> Typo: s/process/processes/
>> Fixed.
>>>> gcov, builds a detailed HTML report and prints a summary, target
>>>> coverage executes LuaJIT-tests and then runs LuaJIT-coverage. Target
>>>> LuaJIT-coverage is useful for building code coverage report for a custom
>>>> set of regression tests.
>>>>
>>>> ```
>>>> $ cmake -S . -B build -DENABLE_COVERAGE=ON
>>>> $ cmake --build build --parallel --target 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 ++++++
>>>>    cmake/CodeCoverage.cmake              | 45 +++++++++++++++++++++++++++
>>>>    test/CMakeLists.txt                   |  7 +++++
>>>>    test/tarantool-c-tests/CMakeLists.txt |  6 +++-
>>>>    4 files changed, 66 insertions(+), 1 deletion(-)
>>>>    create mode 100644 cmake/CodeCoverage.cmake
>>>>
>>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>>> index 6ef24bba..fe6582fa 100644
>>>> --- a/CMakeLists.txt
>>>> +++ b/CMakeLists.txt
>>>> @@ -116,6 +116,15 @@ if(LUAJIT_ENABLE_WARNINGS)
>>>>      )
>>>>    endif()
>>>>    
> I suggest to add comment here, that the user should run tests _before_
> coverage report, or this may be confusing (yes, I'm this user :)):
>
> | $ make LuaJIT-coverage
> | Building coverage report
> | lines: 0.0% (0 out of 23883)
> | functions: 0.0% (0 out of 1765)
> | branches: 0.0% (0 out of 17131)
> | Built target LuaJIT-coverage

The difference for LuaJIT-coverage and coverage targets is described in 
commit message.

Comment is already there:

 >   add_custom_command(TARGET ${PROJECT_NAME}-coverage
 >    COMMENT "Building coverage report"


>
>>>> +set(LUAJIT_ENABLE_COVERAGE_DEFAULT OFF)
>>>> +option(LUAJIT_ENABLE_COVERAGE
>>>> +       "Enable integration with gcovr, a code coverage program"
>>>> +       ${LUAJIT_ENABLE_COVERAGE_DEFAULT})
>>>> +if (LUAJIT_ENABLE_COVERAGE)
>>>> +  AppendFlags(CMAKE_C_FLAGS --coverage)
>>>> +  include(CodeCoverage)
>>>> +endif(LUAJIT_ENABLE_COVERAGE)
>>>> +
>>>>    # Auxiliary flags for main targets (libraries, binaries).
>>>>    AppendFlags(TARGET_C_FLAGS
>>>>      -D_FILE_OFFSET_BITS=64
>>>> diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake
>>>> new file mode 100644
>>>> index 00000000..2be7d129
>>>> --- /dev/null
>>>> +++ b/cmake/CodeCoverage.cmake
>>>> @@ -0,0 +1,45 @@
>>>> +find_program(GCOVR gcovr)
>>>> +find_program(GCOV gcov)
>>>> +
>>>> +set(COVERAGE_DIR "${PROJECT_BINARY_DIR}/coverage")
>>>> +set(COVERAGE_HTML_REPORT "${COVERAGE_DIR}/luajit.html")
>>>> +set(COVERAGE_XML_REPORT "${COVERAGE_DIR}/luajit.xml")
>>>> +
>>>> +if(NOT GCOVR OR NOT GCOV)
>>>> +  add_custom_target(${PROJECT_NAME}-coverage
>>>> +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target"
> I suggest to split this line into several too.

splitted


>
>>>> +  )
>>>> +  message(WARNING "Either `gcovr' or `gcov` not found, \
>>>> +so ${PROJECT_NAME}-coverage target is dummy")
>>> Nit: Something is wrong with alignment here.
>> No, it is intentionally. If you add indentation then these whitespaces
>> will be added to a message.
> Works just fine with the following diff for me:
>
> ===================================================================
> diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake
> index 2be7d129..83e23d7f 100644
> --- a/cmake/CodeCoverage.cmake
> +++ b/cmake/CodeCoverage.cmake
> @@ -9,8 +9,8 @@ if(NOT GCOVR OR NOT GCOV)
>     add_custom_target(${PROJECT_NAME}-coverage
>       COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target"
>     )
> -  message(WARNING "Either `gcovr' or `gcov` not found, \
> -so ${PROJECT_NAME}-coverage target is dummy")
> +  message(WARNING "Either `gcovr' or `gcov` not found, "
> +                  "so ${PROJECT_NAME}-coverage target is dummy")
>     return()
>   endif()
>   
> ===================================================================

Applied, thanks!


>
> <snipped>
>
>>      # Exclude DynASM files, that contain a low-level VM code for CPUs.
>>      --exclude ".*\.dasc"
>>      # Exclude buildvm source code, it's the project's infrastructure.
>>      --exclude ".*/host/"
> Why don't use ${PROJECT_SOURCE_DIR} instead of .* here?


It is not needed here. gcovr searches *.gcda/*.gcno files in 
PROJECT_BINARY_DIRECTORY

and additionally all paths excluded except PROJECT_SOURCE_DIR/src. So 
absolute path is excessive in regexes specified in --exclude options.

>
> <snipped>



>

  reply	other threads:[~2023-08-07 13:39 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-01 18:46 [Tarantool-patches] [PATCH 0/2 v2] Add " Sergey Bronnikov via Tarantool-patches
2023-08-01 18:46 ` [Tarantool-patches] [PATCH 1/2 v2] cmake: add " Sergey Bronnikov via Tarantool-patches
2023-08-02  8:06   ` Maxim Kokryashkin via Tarantool-patches
2023-08-02  8:18     ` Sergey Bronnikov via Tarantool-patches
2023-08-06 11:35       ` Sergey Kaplun via Tarantool-patches
2023-08-07 13:39         ` Sergey Bronnikov via Tarantool-patches [this message]
2023-08-15  8:41           ` Maxim Kokryashkin via Tarantool-patches
2023-08-01 18:46 ` [Tarantool-patches] [PATCH 2/2 v2] ci: support coveralls Sergey Bronnikov via Tarantool-patches
2023-08-02  8:18   ` Maxim Kokryashkin via Tarantool-patches
2023-08-02  8:20     ` Sergey Bronnikov via Tarantool-patches
2023-08-06 11:41     ` Sergey Kaplun via Tarantool-patches
2023-08-07 11:32       ` Sergey Bronnikov via Tarantool-patches
2023-08-21 11:05 ` [Tarantool-patches] [PATCH 0/2 v2] Add code coverage support Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=71807b71-5445-1ce9-f073-a708f87eb29b@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/2 v2] cmake: add code coverage support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox