Re: [Openvpn-devel] [PATCH] tests: Add a simple build sanity check

[Steffan K. <st...@ka...>]

Hi,

On 12-08-17 23:12, David Sommerseth wrote:
> On 12/08/17 12:33, Steffan Karger wrote:
> 
> [...]
> 
>>> +check_option_count()
>>> +{
>>> +    num_min="$1"
>>> +    num_max="$2"
>>> +
>>> +    echo -n "Checking if number of options are between $num_min and $num_max ... "
>>> +    optcount="$(cat sanity_check_options.$$ | wc -l )"
>>> +    if [ $optcount -le $num_min ]; then
>>> +        echo "FAIL  (too few, found $optcount options)"
>>> +        count_failure
>>> +        return
>>> +    fi
>>> +    if [ $optcount -gt $num_max ]; then
>>> +        echo "FAIL  (too many, found $optcount options)"
>>> +        count_failure
>>> +        return
>>> +    fi
>>> +    echo "PASS (found $optcount options)"
>>> +}
>>
>> This is quite fragile.  For example, this breaks 'make check' for
>> --disable-crypto builds.  It will also fail easily after adding or
>> removing some options, and we probably have more configure flags that
>> will cause this check to fail.  That's we I don't like it very much.
> 
> Eeek, so the threshold values are not good enough.  Well, that said, I
> never expected this proposal to get acceptance on the first review :)
> This is just to have a starting point for these checks.
> 
> Based on how the current option parser is designed, it is hard to get
> this 100% correct.  So I think we need to consider a threshold.  For
> example, my system:
> 
> $ openvpn --help | grep -E -- '^--'  | wc -l
> 237
> $ grep -E 'if \(streq\(p\[0\], ".*"\) && ' options.c  | wc -l
> 277
> 
> We might be able to get closer to a realistic number by sending
> options.c via the preprocessor with the right set of "define" arguments.
> 
> Or to rework the complete option parser to be based on a struct-like
> model where it is easy to write a tool which extracts the number of
> options and which options to expect which can be compared against the
> --help output.  This way we could get a close to 100% perfect match.
> But I don't think doing such a code refactoring shoul be based purely on
> the testing requirements in our case.  The code paths involved are quite
> solid and re-used in almost every possible way by OpenVPN (config files,
> PUSH_REPLY, CCD, CCD via management/plug-ins/script hooks, etc).
> 
> Or we could get started on moving the man page over to a more parseable
> file format so we could extract options from the man page and compare it
> to --help.  This way we enforce that the man page is up-to-date too.
> And it could make the man-page be generated according to which features
> OpenVPN is built with.
> 
> Another approach is to make the threshold values (min/max) be based on
> which configure options/defines are enabled - which directly impacts the
> add_option() function in options.c.  So have a baseline of arguments, if
> ENABLE_CRYPTO - add X to the baseline, and so on.
> 
> I do think this test makes sense, as it can ensure 'make check' fails if
> we do something wonky with options.c.  But I agree it is hard to get it
> right.  So just pondering the alternatives and which ones makes most sense.

All this sounds quite complex.  How about splitting this patch in 2
parts: 1) the return value check and 'must always be present' check, and
2) a more elaborate options check.  This way we can have the simple
checks in soon, and can do the more complex checking later.

As for the solution directions for 2), I think the truly elegant way
would be to redo the options parser and add unit tests.  That's a lot
cleaner than a wrapper script.  But also a lot of work...

> That said, I don't buy the argument that testing options can make 'make
> check' fail if adding/removing options.  To me that is similar to say
> unit tests are bad because they will fail when you change the behaviour
> or API of a function which already have a unit test.  So tests will need
> to be adopted according to the changes done on code it is expected to
> test.  But we can ensure doing those changes in the test-case can be
> done in an easily and understandable way.

Fair point.  Though the check should fail as fast as possible, which in
this case means the counts should be strict.  Otherwise, if I add an
option, and 'make check' doesn't fail on my system, it might fail on
other systems.  This is what I mean when I say I think this approach is
fragile.  I'm afraid we will too often find failures only after
commit-and-push.  And that is annoying to maintain.

-Steffan