Re: [Tarantool-patches] [PATCH] cmake: add code coverage support

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Max!


thanks for your comments. See my comments below.


On 7/30/23 16:34, Maxim Kokryashkin wrote:


<snipped>

> Here is the commit message on the branch:
> | cmake: add code coverage support
> |
> | The patch adds building code coverage support using gcovr [1] and gcov.
> | gcovr is a better version of lcov, see [2]. CMake target LuaJIT-coverage
> | executes regression tests, proccess *.gcno and *.gcda files with gcov,
> Typo: s/process/processes/
> | builds detailed HTML report and prints summary about code coverage.
> Typo: s/detailed/a detailed
> Typo: s/summary/a summary/

Fixed.


<snipped>
>>
>>>> +if (ENABLE_COVERAGE)
>>>> +  AppendFlags(CMAKE_C_FLAGS
>>>> +    -fprofile-arcs
>>>> +    -ftest-coverage
>>> Should we use just --coverage?
>> Sure, updated.
> Now the alignments seems kinda strange. I believe it would be
> better to make it a signle line now, like:
> | AppendFlags(CMAKE_C_FLAGS --coverage)

Fixed.

Actually I left it on a separate line intentionally.

This will minimize a diff in case adding more options to CFLAGS.


>>> | --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
> I suggest moving most of the logic with finding gcov/running coverage into
> a separate cmake module, just to make the test-related one more readable.
> What do you think?

Good idea.

Moved to cmake/CodeCoverage.cmake.


<snipped>


>>>> +  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.
> You did not include the actual diff for the change, so I've checked it
> on GitHub. Here are two comments, that were added:
> | # Exclude DynASM files, that contains a low-level VM code for CPUs.
> Typo: s/that contains/that contain/
>
> | # Exclude buildvm source code, it's a project's infrastructure.
> Typo: s/a project's/the project's/


Fixed.

>>
>>>> +        --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?
> Seems adequate to me.

Added:


--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -78,6 +78,9 @@ if(LUAJIT_USE_TEST)
    )

    if (LUAJIT_ENABLE_COVERAGE)
-    add_dependencies(${PROJECT_NAME}-coverage ${PROJECT_NAME}-test)
+    add_custom_target(coverage DEPENDS
+      ${PROJECT_NAME}-test
+      ${PROJECT_NAME}-coverage
+    )
    endif (LUAJIT_ENABLE_COVERAGE)
  endif()


<snipped>
> Maxim Kokryashkin