Re: [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-08-12 12:00+0200, Andrew Jones:
> On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote:
>> On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
>> > 2016-08-10 11:59+1000, Suraj Jitindar Singh:
>> > > 
>> > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> > > @@ -74,6 +74,27 @@ generate_test ()
>> > >  
>> > >  	cat scripts/runtime.bash
>> > >  
>> > > +	if grep -qw "nodefault" <<<${args[1]}; then
>> > > +		echo -e "while true; do\n"\
>> > > +			"\tread -p \"Test marked as not to be run
>> > > by default,"\
>> > > +			"are you sure (Y/N)? \" response\n"\
>> > > +			"\tcase \$response in\n"\
>> > > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
>> > > \"yes\")\n"\
>> > > +			"\t\t\tbreak\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
>> > > +			"\t\t\t;&\n"\
>> > > +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
>> > > +			"\t\t\texit\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\t\t*)\n"\
>> > > +			"\t\t\techo Please select Y or N\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\tesac\n"\
>> > > +			"done"
>> > Uff, this is hard to read.
>> > 
>> > We do not care much about readability of the standalone script
>> > itself,
>> > but the source code should be.  It doesn't have to have be that fancy
>> > with user input either:
>> > 
>> >   echo 'read -p "$question? (y/N)' response
>> >   echo 'case $response in'
>> >   echo '	Y|y|Yes|yes) break;;'
>> >   echo '	*) exit;;
>> >   echo 'esac'
>> > 
>> > It's still ugly, what about adding a function to
>> > scripts/runtime.bash?
>> > More on that below.
>> > 
>> > > 
>> > > +		echo "standalone=\"true\""
>> > We already have $STANDALONE,
>> > 
>> >   echo "export STANDALONE=yes"
>> > 
>> > > 
>> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> > > @@ -48,10 +48,16 @@ function run()
>> > >          return
>> > >      fi
>> > >  
>> > > -    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups;
>> > > then
>> > > +    if [ -n "$only_group" ] && ! grep -qw "$only_group"
>> > > <<<$groups; then
>> > >          return
>> > >      fi
>> > >  
>> > > +    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
>> > > +            ([ -z $standalone ] || [ $standalone != "true" ]);
>> > > then
>> > Continuing the idea about a function:  This can be replaced with
>> > 
>> >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
>> > skip_nodefault;
>> > 
>> > with skip_nodefault defined earlier; It is not a horrible loss to
>> > load
>> > more code in the normal run,
>> > 
>> >   skip_nodefault () {
>> >   	[ "$STANDALONE" != yes ] && return true
>> > 
>> >   	# code ask the question and handle responses -- can be a
>> > fancier
>> >   	# now, that it actually is readable
>> >   }
>> > 
>> > That said, I am not a huge fan of user interaction in tests ...
>> > What is the targeted use-case?
>> The idea was basically to add the option to mark a test as not to
>> be run by default when invoking run_tests.sh. It was then suggested
>> on a previous version of this series that when invoked as a standalone
>> test the user be prompted to confirm that they actually want to
>> run the test.
>> 
>> Since there may be tests which can have a detrimental effect on the
>> host system or some other unintended side effect I thought it better to
>> require the user specifically invoke them.
>> > 
>> > The user has already specifically called this test, ./host_killer, so
>> > asking for confirmation is implying that the user is a monkey.
>> > 
>> > If the test was scripted, then we forced something like
>> > `yes | ./host_killer`.
>> I agree in hindsight that it doesn't make much sense to have the user
>> confirm that they want to run a test that they have specifically
>> invoked. That being said it's possible that someone running it may not
>> know that it has potentially negative effects on the host.
>> 
>> I think it might be better to have tests in the nodefault group require
>> explicit selection by the "-g" parameter when running through
>> run_tests.sh (current effect of series), while when a test is run
>> standalone just run it without any additional user input (different to
>> current operation) and assume the user knows what they are doing. Do
>> you agree with this?
> 
> I disagree. I like the extra protection. The name of the test won't
> be "host-killer", it'll be something like "test-obscure-named-feature".
> The point of standalone tests is to be able to pass them around easily
> and store them for later use. So it's quite likely that the person who
> stores it won't be the person who runs it (or the person who stores it
> will forget what it does by the time they run it) Anybody who wants to
> avoid the prompt can simply wrap the standalone script in another one
> 
> cat <<EOF > set-trap-for-unsuspecting-users
> #/bin/bash
> yes | ./test-obscure-named-feature
> EOF

Ok, experience with `yum` made me tolerant. :)
I would go with the check inside scripts/runtime.bash then.

> We could also add a couple standard options to standalone tests,
> -h (help - output what the test does, warn about crashing hosts, etc.)

Sounds nice.
Could also work with `./run_tests.sh -h` to print them all.

> -y (yes  - say yes at any prompts)

What about adding a "-g $group" option to standalone tests instead.?

We could then use

  for test in tests/*; do $test -g $group; done

to run the same tests as

  ./run_test.sh -g $group

> -h would take its text from the unittests.cfg file (we'd add a new
> unit test property called 'help' there)