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