Re: [kvm-unit-tests PATCH V3 1/5] 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>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com,
	kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com
Subject: Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default
Date: Wed, 17 Aug 2016 13:14:33 +1000	[thread overview]
Message-ID: <1471403673.4989.1.camel@gmail.com> (raw)
In-Reply-To: <20160816120037.32l5sezhhsgtldxa@kamzik.localdomain>

On Tue, 2016-08-16 at 14:00 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Invoking run_tests.sh without the -g parameter will by default run
> > all of
> > the tests for a given architecture. This patch series will add a
> > test which
> > has the ability to bring down the host and thus it might be nice if
> > we
> > double check that the user actually wants to run that test instead
> > of
> > them unknowingly bringing down a machine they might not want to.
> > 
> > In order to do this add the option for a tests' group parameter in
> > unittests.cfg to include "nodefault" on order to indicate that it
> > shouldn't
> > be run be default.
> > 
> > When tests are invoked via run_tests.sh those with the nodefault
> > group
> > parameter will be skipped unless explicitly specified by the "-g"
> > command
> > line option. When tests with the nodefault group parameter are
> > built and
> > run standalone the user will be prompted on invocation to confirm
> > that
> > they actually want to run the test.
> > 
> > This allows a developer to mark a test as having potentially
> > adverse
> > effects and thus requires an extra level of confirmation from the
> > user
> > before they are invoked. Existing functionality will be preserved
> > and new
> > tests can choose any group other than "nodefault" if they want to
> > be run
> > by default.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Move checking on standalone invokation into a function
> > 	  "skip_nodefault" in scripts/runtime.bash
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  arm/unittests.cfg       |  3 +++
> >  powerpc/unittests.cfg   |  3 +++
> >  scripts/mkstandalone.sh |  4 ++++
> >  scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
> >  x86/unittests.cfg       |  3 +++
> >  5 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index ffd12e5..3f6fa45 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index ed4fdbe..0098cb6 100644
> > --- a/powerpc/unittests.cfg
> > +++ b/powerpc/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > index d2bae19..b921416 100755
> > --- a/scripts/mkstandalone.sh
> > +++ b/scripts/mkstandalone.sh
> > @@ -74,6 +74,10 @@ generate_test ()
> >  
> >  	cat scripts/runtime.bash
> >  
> > +	if grep -qw "nodefault" <<<${args[1]}; then
> > +		echo "export STANDALONE=yes"
> > +	fi
> > +
> The answer is 42. Or, rather, we already unconditionally do this
> on line 42 of this file, so this hunk isn't needed.
Woops, missed that.
> 
> > 
> >  	echo "run ${args[@]}"
> >  }
> >  
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > index 0503cf0..383ebd4 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -32,6 +32,26 @@ get_cmdline()
> >      echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel
> > $RUNTIME_arch_run $kernel -smp $smp $opts"
> >  }
> >  
> > +skip_nodefault()
> > +{
> > +    [ "$STANDALONE" != yes ] && return 0
> For consistency throughout our scripts, please quote all strings,
> like "yes"
> 
> > 
> > +
> > +    while true; do
> > +        read -p "Test marked not to be run by default, are you
> > sure (Y/N)? " yn
> > +        case $yn in
> > +            "Y" | "y" | "Yes" | "yes")
> What about "YES" :-)
> 
> Actually, I'd just accept 'Y' for yes, and nothing else, like the
> prompt says.
> And, instead of looping for valid input, all other input can just
> mean no.
> 
> > 
> > +                return 1
> > +                ;;
> > +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> We should output something when the answer is 'no' like "User
> aborted",
> or whatever.
> 
> > 
> > +                exit
> > +                ;;
> > +            *)
> > +                echo Please select Y or N
> > +                ;;
> > +        esac
> > +    done
> > +}
> > +
> >  function run()
> >  {
> >      local testname="$1"
> > @@ -48,10 +68,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 &&
> > +            skip_nodefault; then
> > +        echo -e "`SKIP` $testname (test marked as manual run
> > only)"
> > +        return;
> > +    fi
> > +
> >      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
> >          echo "`SKIP` $1 ($arch only)"
> >          return 2
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 60747cf..4a1f74e 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.