On 12/08/17 12:33, Steffan Karger wrote: [...] >> --- >> tests/Makefile.am | 2 +- >> tests/t_sanity_check.sh | 118 >> ++++++++++++++++++++++++++++++++++++++++++++++++ > > t_sanity_check is less descriptive than the t_usage proposed by Ilya. > (Sanity check could be anything, while we specifically test the usage > output.)
Fair point. I tried to avoid t_usage, as it also checks for a segfault
(which is where this all started). But when thinking of it, it does
strictly tests usage*() for segfault. I can switch back to t_usage.
[...]
>> +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.
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.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
