From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Date: Wed, 17 Aug 2016 03:14:33 +0000 Subject: Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default Message-Id: <1471403673.4989.1.camel@gmail.com> List-Id: References: <1471331895-29887-1-git-send-email-sjitindarsingh@gmail.com> <20160816120037.32l5sezhhsgtldxa@kamzik.localdomain> In-Reply-To: <20160816120037.32l5sezhhsgtldxa@kamzik.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Andrew Jones Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com 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 > > --- > > > > Change Log: > > > > V2 -> V3: > > - Move checking on standalone invokation into a function > >   "skip_nodefault" in scripts/runtime.bash > > > > Signed-off-by: Suraj Jitindar Singh > > --- > >  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 = ... # 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/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 = ... # 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh 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 Message-ID: <1471403673.4989.1.camel@gmail.com> References: <1471331895-29887-1-git-send-email-sjitindarsingh@gmail.com> <20160816120037.32l5sezhhsgtldxa@kamzik.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com To: Andrew Jones Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:33935 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751499AbcHQDOl (ORCPT ); Tue, 16 Aug 2016 23:14:41 -0400 In-Reply-To: <20160816120037.32l5sezhhsgtldxa@kamzik.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > --- > > > > Change Log: > > > > V2 -> V3: > > - Move checking on standalone invokation into a function > >   "skip_nodefault" in scripts/runtime.bash > > > > Signed-off-by: Suraj Jitindar Singh > > --- > >  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 = ... # 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/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 = ... # 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.