From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [kvm-unit-tests PATCH 1/4] scripts/runtime: Add ability to mark test as don't run by default Date: Fri, 12 Aug 2016 11:25:55 +0200 Message-ID: <20160812092555.d4l7vyvmlhibk5gu@kamzik.localdomain> References: <1470382393-24209-1-git-send-email-sjitindarsingh@gmail.com> <20160805081814.6empeh5vvwiw2dre@hawk.localdomain> <1470628027.7116.9.camel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, pbonzini@redhat.com To: Suraj Jitindar Singh Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45790 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751258AbcHLJZ7 (ORCPT ); Fri, 12 Aug 2016 05:25:59 -0400 Content-Disposition: inline In-Reply-To: <1470628027.7116.9.camel@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Aug 08, 2016 at 01:47:07PM +1000, Suraj Jitindar Singh wrote: > 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 > > > --- > > >  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 = ... # 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 = ... # 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. Sorry for the slow response. I've been on vacation. Yes, I meant the later, when the user runs the test she'll get prompted to continue for 'nodefault' type tests. Thanks, drew > > > > > > > > +        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 = ... # 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. > -- > 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