[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