[SSSD] [PATCH v5] Add basic support for CI test execution
Michal Židek
mzidek at redhat.com
Mon Sep 1 17:27:57 UTC 2014
On 09/01/2014 05:51 PM, Nikolai Kondrashov wrote:
> Hi Michal,
>
> Thanks a lot for the review! My answers are below.
>
> On 09/01/2014 06:15 PM, Michal Židek wrote:
>> On 09/01/2014 01:47 PM, Nikolai Kondrashov wrote:
>>> On 08/09/2014 09:26 PM, Nikolai Kondrashov wrote:
>>>> Here is the fifth version of the patch adding CI scripts.
>>>
>>> This is the version of CI patches I would like merged, when you'll find
>>
>> I have some comments.
>>
>> 1. Valgrind:
>> The valgrind check needs some enhancements. Currently the results of
>> this step are just ignored (the test suite fails and this failure is
>> ignored). In the script there is commnet:
>> # Ignored until issues found by Valgrind are fixed
>> But this will NEVER happen.
>>
>> If we want to get something out of this step than we need
>> to know at least:
>> a) how many tests failed,
>> b) how many valgrind errors were detected
>> and this information must be printed on the screen (not hidden in the
>> logs, otherwise everyone will just ignore it). This way, we will know,
>> that no new errors/failures were introduced. And if the number rises
>> than we check if it is a false positive or not in the logs.
>
> Ignoring Valgrind issues doesn't of course make our tests verified by
> Valgrind. However, it provides an easy way to run the tests under
> Valgrind and
> CI setup allows running them on different distros where they could be
> slightly
> different. Overall, this is a good way to start working on Valgrind
issues.
> This way anybody having interest in fixing particular code will be
able to
> easily get Valgrind results.
>
> Now, how to fix it is another matter. Some issues can definitely be
fixed.
> Some might be too difficult to fix quickly, others might be false
> positives.
> So I suggest we start fixing the easier issues and suppress others, at
> least
> until better time. Until we get to the state where that ignoring can be
> removed and we get binary status: PASSED or FAILED. PASSED meaning zero
> non-suppressed errors, FAILED meaning at least one. I plan to start
> doing just
> that after we get these patches in.
>
> Having a count of errors wouldn't be much better than ignoring the
results.
>
>> 2. Clang-static analyser:
>> Similar to the valgrind step. We at least need some numbers to be
>> printed on the screen (number of problems detected), otherwise this
>> step will be very little useful and basically will just make the
>> whole "check" longer.
>
> My response and plan here would be similar to the Valgrind's case. Only
> here
> we may perhaps disable certain error classes for the start, or use Clang
> augmentation in the code, making ignore the difficult issues, but in the
> end
> we should arrive to the same binary status.
>
About Valgrind and Clang, I think you underestimate the amount of time
that is in front of us before we get to the binary PASS/FAIL state.
Until then, these checks are completely useless and only take time
(which is one of the reasons why I wanted to be able to disable/enable
specific steps).
The number would tell us if something changed from the past run or not.
(and would make these steps less useless). It would be a valuable
improvement IMO.
Also I do not think disabling specific category of test in static
analyser is a good idea.
>> 3. More options:
>> I would like to have possibility to disable/enable specific parts.
>> The -e/-m/-r flags are IMO not sufficient (for example as long as we
>> do not fix 1 and 2, I would like to disable/not enable these parts
>> using some argument)
>
> I would like that as well. That's why I would like to try using Epoxy
- my
> Bash test framework (https://github.com/spbnick/epoxy), which allows
> just that
> and more.
I am not familiar with epoxy, but Having better framework for shell
script based tests would be a nice thing. But I would like to stick to
programming languages our project already depends on (python/C/shell). I
see that epoxy depends on Lua. Python framework would be better
for us IMO.
>
> How soon I'll start working on that depends on how much pain the current
> limited flexibility will cause me and others. If Epoxy use is rejected,
> I'll
> fallback to writing support for this by hand, but I wouldn't like to do
> that
> kind of work all over again.
>
> Thanks for the review again, Michal!
>
> Nick
More information about the sssd-devel
mailing list