Re: [Openvpn-devel] [PATCH] tests: Add a simple build sanity check
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
Re: [Openvpn-devel] [PATCH] tests: Add a simple build sanity check
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
Re: [Openvpn-devel] [PATCH] tests: Add a simple build sanity check
Hi,
Some high-level comments:
On 09-08-17 14:32, David Sommerseth wrote:
> This runs openvpn --help to check if the output is somewhat
> sensible and sane. It will catch if the binary segfaults,
> if it is a normal build or an --enable-small build and
> does some simple checks when a list of options is produced.
>
> This is based on the discussions in this [1] mailing thread.
>
> [1]
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15172.html
> Message-Id: <20170807132301.22759-3-chipits...@gmail.com>
While extensive testing is good, fragile tests are not so good. I'm
afraid this patch crosses that line.
> Signed-off-by: David Sommerseth
> ---
> 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.)
> 2 files changed, 119 insertions(+), 1 deletion(-)
> create mode 100755 tests/t_sanity_check.sh
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 0795680c..7af5101e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -14,7 +14,7 @@ MAINTAINERCLEANFILES = \
>
> SUBDIRS = unit_tests
>
> -test_scripts = t_client.sh
> +test_scripts = t_sanity_check.sh t_client.sh
> if ENABLE_CRYPTO
> test_scripts += t_lpback.sh t_cltsrv.sh
> endif
> diff --git a/tests/t_sanity_check.sh b/tests/t_sanity_check.sh
> new file mode 100755
> index ..e6c228c8
> --- /dev/null
> +++ b/tests/t_sanity_check.sh
> @@ -0,0 +1,118 @@
> +#! /bin/sh
> +#
> +# t_sanity_check.sh -- Check that openvpn --help makes somewhat sense
> +#
> +# Copyright (C) 2017 David Sommerseth
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License
> +# as published by the Free Software Foundation; either version 2
> +# of the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +# 02110-1301, USA.
> +
> +set -u
> +top_builddir="${top_builddir:-..}"
> +
> +failed=0
> +count_failure()
> +{
> +failed=$(($failed + 1))
> +}
> +
> +
> +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.
> +
> +check_options_present()
> +{
> +for opt in $*;
> +do
> +echo -n "Checking for option --${opt} ..."
> +grep -E "^--${opt} " sanity_check_options.$$ 1>/dev/null 2>&1
> +if [ $? -ne 0 ]; then
> +echo "FAIL (missing option)"
> +count_failure
> +else
> +echo "PASS"
> +fi
> +done
> +}
> +
> +echo "*** OpenVPN sanity check: openvpn --help"
> +echo -n "Running 'openvpn --help' ... "
> +"${top_builddir}/src/openvpn/openvpn" --help > sanity_check_log.$$ 2>&1
> +res=$?
> +if [ $res -ne 1 ]; then
> +echo "FAIL (Something bad happened)"
> +cat sanity_check_log.$$
> +count_failure
> +else
> +echo "PASS"
> +echo -n "Check build type ... "
> +linecount="$(cat sanity_check_log.$$ | wc -l)"
> +if [ $linecount -eq 1 ]; then
> +# Is this an --enable-small build?
> +grep "Usage message not available" sanity_check_log.$$ \
> +1> /dev/null 2> /dev/null
> +if [ $? -ne 0 ]; then
> +echo "Unknown build type"
> +cat sanity_check_log.$$
> +count_failure
> +else
> +echo "PASS (--enable-small build, no further checks)"
> +fi
> +else
> +echo "PASS (normal build)"
> +
> +# Extract only the options
> +echo -n "Extracting options ... "
> +grep -E -- ^-- sanity_check_log.$$ > sanity_check_options.$$
> +if [ $? -ne 0 ]; then
> +echo "F
