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