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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

On Fri, 2016-08-05 at 10:18 +0200, Andrew Jones wrote:
> On Fri, Aug 05, 2016 at 05:33:10PM +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 be set as "nodefault" on order to indicate that
> > it shouldn't be run be default. Modify runtime.bash such that if
> > one of
> > these tests is encountered a message will be printed to the user to
> > indicate that the task was skipped and with instructions on how to
> > run
> > the test on it's own.
> > 
> > This allows a user to confirm that they want to run a test which
> > has been
> > marked as not to be run by default for whatever reason by the
> > creator.
> > 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>
> > ---
> >  arm/unittests.cfg     |  3 +++
> >  powerpc/unittests.cfg |  3 +++
> >  scripts/runtime.bash  | 10 ++++++++++
> >  x86/unittests.cfg     |  3 +++
> >  4 files changed, 19 insertions(+)
> > 
> > I was going to have the long error message gated behind
> > [ $verbose == "yes" ] but this means that when that test is run
> > standalone
> > it will skip without any obvious indication as to why.
> > Thus IMO it's better to have the message printed regardless.
> > 
> > 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/runtime.bash b/scripts/runtime.bash
> > index 0503cf0..6bf28fb 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -52,6 +52,16 @@ function run()
> >          return
> >      fi
> >  
> > +    if grep -q "nodefault" <<<$groups && ! grep -qw "$only_group"
> > <<<$groups; then
> I think we want if [ -z "$only_group" ] && grep -qw "nodefault"
> <<<$groups
> The condition above already checks that if $only_group is given that
> it's
> in $groups. Also, please add -w to the grep in that condition above.
That would make more sense, I'll change this.
> 
> > 
> > +        echo -e "`SKIP` $testname\n" \
> > +            "Test $testname marked as not to be run by default,\n"
> > \
> > +            "please ensure that you actually want to run this
> > test\n" \
> > +            "To run this using run_tests.sh append \"-g
> > $groups\"\n" \
> > +            "To run this standalone set the only_group
> > parameter\n" \
> > +            "\"only_group=$groups tests/$testname\""
> We prefer one line summaries be output here. How about just adding
> "(manual run only - host may crash)" or some such, to the SKIP line.
Ok, I'll make it something like "test marked manual run only" as
this
may have been done for reasons other than the test can bring
down the
host.
> As for the 'only_group=$groups tests/$testname' standalone
> instructions.
> I think mkstandalone should be modified to check for nodefault and
> output a message saying only continue if you're sure, and then wait
> for
> input from the user to continue.
That's a more logical option, this was just the easiest. I'll look
into doing something like this.
When you say mkstandalone should be modified do you mean when
the user runs "make standalone" to build the tests they should be
prompted or when they actually invoke the test they are asked
to confirm that they actually want to run it? Having the
confirmation step on actual test invokation makes more sense IMO.
> 
> > 
> > +        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.