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: "Andrew Jones" <drjones@redhat.com>, "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
Subject: Re: [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default
Date: Mon, 15 Aug 2016 09:41:07 +1000	[thread overview]
Message-ID: <1471218067.4052.8.camel@gmail.com> (raw)
In-Reply-To: <20160812125804.eomqc56uoik6ju74@kamzik.localdomain>

On Fri, 2016-08-12 at 14:58 +0200, Andrew Jones wrote:
> On Fri, Aug 12, 2016 at 02:06:36PM +0200, Radim Krčmář wrote:
> > 
> > 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.
> Sounds good.
> 
> > 
> > 
> > > 
> > > -y (yes  - say yes at any prompts)
> > What about adding a "-g $group" option to standalone tests
> > instead.?
> I'd rather the concept of group disappear for standalone tests. IMO,
> a standalone test isn't a member of a group or of a test framework.
> It's just a script with an embedded binary.
I agree that it'd be better to keep the idea of groups and standalone
tests separate. You shouldn't have to worry about groups when running a
standalone test.
> 
> > 
> > 
> > We could then use
> > 
> >   for test in tests/*; do $test -g $group; done
> > 
> > to run the same tests as
> > 
> >   ./run_test.sh -g $group
> Being able to run all standalone tests in a group isn't a bad idea,
> but to keep the standalone test feel we could provide generated
> scripts
> named group-name.sh that does the above. IOW, I'm OK with adding -g
> support to standalone scripts if it stays hidden within another
> "just a script"
The idea of being able to run all tests in a given group makes sense.
Although we could see the case with some overlap if a test subscribes
to many different groups, although currently this isn't the case with
any existing tests and may never be the case.
> 
> Suraj,
> 
> IMO, you don't need to worry about these ideas (-h, -y, group-
> name.sh)
> for this series. We can do those later. However I'm happy to review
> anything you pull together along these lines :-)
Alright, for this series I think I'll move the checking into a function
in scripts/runtime.bash and call it a day.

Then I'll look at putting some of the -h and -y functionality into
another series.
> 
> Thanks,
> drew
> 
> > 
> > 
> > > 
> > > -h would take its text from the unittests.cfg file (we'd add a
> > > new
> > > unit test property called 'help' there)
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html