| Message ID | 1471331895-29887-1-git-send-email-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. > -- > 2.5.5 > Thanks, drew -- 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
2016-08-16 14:00+0200, Andrew Jones: > On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote: >> diff --git a/scripts/runtime.bash b/scripts/runtime.bash >> @@ -32,6 +32,26 @@ get_cmdline() >> +skip_nodefault() >> +{ >> + while true; do >> + read -p "Test marked not to be run by default, are you sure (Y/N)? " yn I'd write "run this test" instead of "are you sure", or something similar for the question. The user can be sure that the has is marked with nodefault. ;) >> + case $yn in >> + "Y" | "y" | "Yes" | "yes") > > What about "YES" :-) And exclamation marks! "YES!!!" > Actually, I'd just accept 'Y' for yes, and nothing else, like the prompt says. NO. If it is only one value, then make it "y". motto: saving the Earth, one shift at a time. This kind of user interface usually accepts at least "[yY]|[yY]es" ... users will already be pissed that they have to input something and denying a perfectly logical "yes" (which is what "y" stands for) is going too overboard, IMO. > And, instead of looping for valid input, all other input can just mean no. "y/N" is the convention for writing a bool question that defaults to no. I'd accept "" (just enter) as the default and then, looping isn't unexpected and user already typed some crap in that case, so they probably want to answer the question without having to run the command again. >> + return 1 >> + ;; >> + "N" | "n" | "No" | "no" | "q" | "quit" | "exit") > > We should output something when the answer is 'no' like "User aborted", > or whatever. > >> + exit Wouldn't "return 1" and the SKIP message be enough? >> + ;; >> + *) >> + echo Please select Y or N Just asking the question again is more common -- it's not hard to figure out that the answer was not accepted. -- 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
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. -- 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
On Tue, 2016-08-16 at 18:03 +0200, Radim Krčmář wrote: > 2016-08-16 14:00+0200, Andrew Jones: > > > > On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh > > wrote: > > > > > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > > > @@ -32,6 +32,26 @@ get_cmdline() > > > +skip_nodefault() > > > +{ > > > + while true; do > > > + read -p "Test marked not to be run by default, are you > > > sure (Y/N)? " yn > I'd write "run this test" instead of "are you sure", or something > similar for the question. The user can be sure that the has is > marked > with nodefault. ;) I'll go with "run this test?" > > > > > > > > > + case $yn in > > > + "Y" | "y" | "Yes" | "yes") > > What about "YES" :-) > And exclamation marks! > > "YES!!!" > > > > > Actually, I'd just accept 'Y' for yes, and nothing else, like the > > prompt says. > NO. If it is only one value, then make it "y". > motto: saving the Earth, one shift at a time. > > This kind of user interface usually accepts at least "[yY]|[yY]es" > ... > users will already be pissed that they have to input something and > denying a perfectly logical "yes" (which is what "y" stands for) is > going too overboard, IMO. > > > > > And, instead of looping for valid input, all other input can just > > mean no. > "y/N" is the convention for writing a bool question that defaults to > no. > I'd accept "" (just enter) as the default and then, looping isn't > unexpected and user already typed some crap in that case, so they > probably want to answer the question without having to run the > command > again. I'd rather keep the "[yY]|[yY]es" options it doesn't really make sense not to. I can see the argument for having all other input default to no. You really see a mix of the two in the wild where sometimes junk will default to no and sometimes it will loop and ask again. I tend to think that most of the time when it doesn't match one of the options it's going to be because of a mis-type by the user and they'd probably prefer that it looped than have to run the command again. > > > > > > > > > + return 1 > > > + ;; > > > + "N" | "n" | "No" | "no" | "q" | "quit" | "exit") > > We should output something when the answer is 'no' like "User > > aborted", > > or whatever. I don't really see this as being necessary, I guess there could be some ambiguity as to what has happened but I think it's pretty obvious that the test has aborted. I'll change it so that the skip message is printed. > > > > > > > > + exit > Wouldn't "return 1" and the SKIP message be enough? Yeah if I have return 0 here (because bash things) then it'll print the skip message and abort the test. > > > > > > > > > + ;; > > > + *) > > > + echo Please select Y or N > Just asking the question again is more common -- it's not hard to > figure > out that the answer was not accepted. Ok, I've seen places where the options are specified like this but will change it to just have the same question again. -- 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
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 + 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 + + 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") + return 1 + ;; + "N" | "n" | "No" | "no" | "q" | "quit" | "exit") + 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.