From: Andrew Jones <drjones@redhat.com> To: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com>, kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com 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 12:00:37 +0200 [thread overview] Message-ID: <20160812100037.m2sobctrjgrrsj5t@kamzik.localdomain> (raw) In-Reply-To: <1470982393.4695.9.camel@gmail.com> On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote: > 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? I disagree. I like the extra protection. The name of the test won't be "host-killer", it'll be something like "test-obscure-named-feature". The point of standalone tests is to be able to pass them around easily and store them for later use. So it's quite likely that the person who stores it won't be the person who runs it (or the person who stores it will forget what it does by the time they run it) Anybody who wants to avoid the prompt can simply wrap the standalone script in another one cat <<EOF > set-trap-for-unsuspecting-users #/bin/bash yes | ./test-obscure-named-feature EOF We could also add a couple standard options to standalone tests, -h (help - output what the test does, warn about crashing hosts, etc.) -y (yes - say yes at any prompts) -h would take its text from the unittests.cfg file (we'd add a new unit test property called 'help' there) Thanks, drew > > > > > > > > + echo -e "`SKIP` $testname - (test marked as manual run > > > only)" > > Please remove the whitespaced dash " - " from output. > > > Will Fix > > Thanks. > Thanks for the comments > -- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Jones <drjones@redhat.com> To: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com>, kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com 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 10:00:37 +0000 [thread overview] Message-ID: <20160812100037.m2sobctrjgrrsj5t@kamzik.localdomain> (raw) In-Reply-To: <1470982393.4695.9.camel@gmail.com> On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote: > 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? I disagree. I like the extra protection. The name of the test won't be "host-killer", it'll be something like "test-obscure-named-feature". The point of standalone tests is to be able to pass them around easily and store them for later use. So it's quite likely that the person who stores it won't be the person who runs it (or the person who stores it will forget what it does by the time they run it) Anybody who wants to avoid the prompt can simply wrap the standalone script in another one cat <<EOF > set-trap-for-unsuspecting-users #/bin/bash yes | ./test-obscure-named-feature EOF We could also add a couple standard options to standalone tests, -h (help - output what the test does, warn about crashing hosts, etc.) -y (yes - say yes at any prompts) -h would take its text from the unittests.cfg file (we'd add a new unit test property called 'help' there) Thanks, drew > > > > > > > > + echo -e "`SKIP` $testname - (test marked as manual run > > > only)" > > Please remove the whitespaced dash " - " from output. > > > Will Fix > > Thanks. > Thanks for the comments > -- > 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
next prev parent reply other threads:[~2016-08-12 10:00 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-10 1:59 [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default Suraj Jitindar Singh 2016-08-10 1:59 ` Suraj Jitindar Singh 2016-08-10 1:59 ` [kvm-unit-tests PATCH V2 2/4] lib/powerpc: Add generic decrementer exception handler Suraj Jitindar Singh 2016-08-10 1:59 ` Suraj Jitindar Singh 2016-08-10 10:38 ` Thomas Huth 2016-08-10 10:38 ` Thomas Huth 2016-08-12 6:17 ` Suraj Jitindar Singh 2016-08-12 6:17 ` Suraj Jitindar Singh 2016-08-10 1:59 ` [kvm-unit-tests PATCH V2 3/4] lib/powerpc: Add function to start secondary threads Suraj Jitindar Singh 2016-08-10 1:59 ` Suraj Jitindar Singh 2016-08-10 11:25 ` Thomas Huth 2016-08-10 11:25 ` Thomas Huth 2016-08-12 6:30 ` Suraj Jitindar Singh 2016-08-12 6:30 ` Suraj Jitindar Singh 2016-08-12 11:19 ` Thomas Huth 2016-08-12 11:19 ` Thomas Huth 2016-08-15 1:01 ` Suraj Jitindar Singh 2016-08-15 1:01 ` Suraj Jitindar Singh 2016-08-12 17:07 ` Andrew Jones 2016-08-12 17:07 ` Andrew Jones 2016-08-15 1:58 ` Suraj Jitindar Singh 2016-08-15 1:58 ` Suraj Jitindar Singh 2016-08-15 6:27 ` Andrew Jones 2016-08-15 6:27 ` Andrew Jones 2016-08-16 5:10 ` Suraj Jitindar Singh 2016-08-16 5:10 ` Suraj Jitindar Singh 2016-08-10 1:59 ` [kvm-unit-tests PATCH V2 4/4] powerpc/tm: Add a test for H_CEDE while tm suspended Suraj Jitindar Singh 2016-08-10 1:59 ` Suraj Jitindar Singh 2016-08-10 9:43 ` Thomas Huth 2016-08-10 9:43 ` Thomas Huth 2016-08-12 6:36 ` Suraj Jitindar Singh 2016-08-12 6:36 ` Suraj Jitindar Singh 2016-08-10 11:33 ` Thomas Huth 2016-08-10 11:33 ` Thomas Huth 2016-08-12 6:36 ` Suraj Jitindar Singh 2016-08-12 6:36 ` Suraj Jitindar Singh 2016-08-12 17:19 ` Andrew Jones 2016-08-12 17:19 ` Andrew Jones 2016-08-15 2:01 ` Suraj Jitindar Singh 2016-08-15 2:01 ` Suraj Jitindar Singh 2016-08-10 13:22 ` [kvm-unit-tests PATCH V2 1/4] scripts/runtime: Add ability to mark test as don't run by default Radim Krčmář 2016-08-10 13:22 ` Radim Krčmář 2016-08-12 6:13 ` Suraj Jitindar Singh 2016-08-12 6:13 ` Suraj Jitindar Singh 2016-08-12 10:00 ` Andrew Jones [this message] 2016-08-12 10:00 ` Andrew Jones 2016-08-12 12:06 ` Radim Krčmář 2016-08-12 12:06 ` Radim Krčmář 2016-08-12 12:58 ` Andrew Jones 2016-08-12 12:58 ` Andrew Jones 2016-08-14 23:41 ` Suraj Jitindar Singh 2016-08-14 23:41 ` Suraj Jitindar Singh
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160812100037.m2sobctrjgrrsj5t@kamzik.localdomain \ --to=drjones@redhat.com \ --cc=kvm-ppc@vger.kernel.org \ --cc=kvm@vger.kernel.org \ --cc=lvivier@redhat.com \ --cc=pbonzini@redhat.com \ --cc=rkrcmar@redhat.com \ --cc=sjitindarsingh@gmail.com \ --cc=thuth@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.