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

From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
To: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com,
	kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com,
	drjones@redhat.com
Subject: Re: [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default
Date: Fri, 12 Aug 2016 16:13:13 +1000	[thread overview]
Message-ID: <1470982393.4695.9.camel@gmail.com> (raw)
In-Reply-To: <20160810132205.GA1574@potion>

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?
> 
> > 
> > +        echo -e "`SKIP` $testname - (test marked as manual run
> > only)"
> Please remove the whitespaced dash " - " from output.
> 
Will Fix
> Thanks.
Thanks for the comments