From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default Date: Fri, 12 Aug 2016 16:13:13 +1000 Message-ID: <1470982393.4695.9.camel@gmail.com> References: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com> <20160810132205.GA1574@potion> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com, drjones@redhat.com To: Radim =?UTF-8?Q?Kr=C4=8Dm=C3=A1=C5=99?= Return-path: Received: from mail-pa0-f52.google.com ([209.85.220.52]:34280 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751356AbcHLGNj (ORCPT ); Fri, 12 Aug 2016 02:13:39 -0400 In-Reply-To: <20160810132205.GA1574@potion> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote: > 2016-08-10 11:59+1000, Suraj Jitindar Singh: > > > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > > @@ -74,6 +74,27 @@ generate_test () > >   > >   cat scripts/runtime.bash > >   > > + if grep -qw "nodefault" <<<${args[1]}; then > > + echo -e "while true; do\n"\ > > + "\tread -p \"Test marked as not to be run > > by default,"\ > > + "are you sure (Y/N)? \" response\n"\ > > + "\tcase \$response in\n"\ > > + "\t\t\"Y\" | \"y\" | \"Yes\" | > > \"yes\")\n"\ > > + "\t\t\tbreak\n"\ > > + "\t\t\t;;\n"\ > > + "\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\ > > + "\t\t\t;&\n"\ > > + "\t\t\"q\" | \"quit\" | \"exit\")\n"\ > > + "\t\t\texit\n"\ > > + "\t\t\t;;\n"\ > > + "\t\t*)\n"\ > > + "\t\t\techo Please select Y or N\n"\ > > + "\t\t\t;;\n"\ > > + "\tesac\n"\ > > + "done" > Uff, this is hard to read. > > We do not care much about readability of the standalone script > itself, > but the source code should be.  It doesn't have to have be that fancy > with user input either: > >   echo 'read -p "$question? (y/N)' response >   echo 'case $response in' >   echo ' Y|y|Yes|yes) break;;' >   echo ' *) exit;; >   echo 'esac' > > It's still ugly, what about adding a function to > scripts/runtime.bash? > More on that below. > > > > > + echo "standalone=\"true\"" > We already have $STANDALONE, > >   echo "export STANDALONE=yes" > > > > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > > @@ -48,10 +48,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 && > > +            ([ -z $standalone ] || [ $standalone != "true" ]); > > then > Continuing the idea about a function:  This can be replaced with > >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups && > skip_nodefault; > > with skip_nodefault defined earlier; It is not a horrible loss to > load > more code in the normal run, > >   skip_nodefault () { >    [ "$STANDALONE" != yes ] && return true > >    # code ask the question and handle responses -- can be a > fancier >    # now, that it actually is readable >   } > > That said, I am not a huge fan of user interaction in tests ... > What is the targeted use-case? The idea was basically to add the option to mark a test as not to be run by default when invoking run_tests.sh. It was then suggested on a previous version of this series that when invoked as a standalone test the user be prompted to confirm that they actually want to run the test. Since there may be tests which can have a detrimental effect on the host system or some other unintended side effect I thought it better to require the user specifically invoke them. > > The user has already specifically called this test, ./host_killer, so > asking for confirmation is implying that the user is a monkey. > > If the test was scripted, then we forced something like > `yes | ./host_killer`. I agree in hindsight that it doesn't make much sense to have the user confirm that they want to run a test that they have specifically invoked. That being said it's possible that someone running it may not know that it has potentially negative effects on the host. I think it might be better to have tests in the nodefault group require explicit selection by the "-g" parameter when running through run_tests.sh (current effect of series), while when a test is run standalone just run it without any additional user input (different to current operation) and assume the user knows what they are doing. Do you agree with this? > > > > > +        echo -e "`SKIP` $testname - (test marked as manual run > > only)" > Please remove the whitespaced dash " - " from output. > Will Fix > Thanks. Thanks for the comments