[SSSD] [PATCH v5] Add basic support for CI test execution

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Mon Sep 1 15:51:52 UTC 2014


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.

> 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.

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