From: "Radim Krčmář" <rkrcmar@redhat.com> To: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com, drjones@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: Wed, 10 Aug 2016 15:22:06 +0200 [thread overview] Message-ID: <20160810132205.GA1574@potion> (raw) In-Reply-To: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com> 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 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`. > + echo -e "`SKIP` $testname - (test marked as manual run only)" Please remove the whitespaced dash " - " from output. Thanks.
WARNING: multiple messages have this Message-ID (diff)
From: "Radim Krčmář" <rkrcmar@redhat.com> To: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com, drjones@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: Wed, 10 Aug 2016 13:22:06 +0000 [thread overview] Message-ID: <20160810132205.GA1574@potion> (raw) In-Reply-To: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com> 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 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`. > + echo -e "`SKIP` $testname - (test marked as manual run only)" Please remove the whitespaced dash " - " from output. Thanks.
next prev parent reply other threads:[~2016-08-10 18:08 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 ` Radim Krčmář [this message] 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-12 6:13 ` Suraj Jitindar Singh 2016-08-12 6:13 ` Suraj Jitindar Singh 2016-08-12 10:00 ` Andrew Jones 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=20160810132205.GA1574@potion \ --to=rkrcmar@redhat.com \ --cc=drjones@redhat.com \ --cc=kvm-ppc@vger.kernel.org \ --cc=kvm@vger.kernel.org \ --cc=lvivier@redhat.com \ --cc=pbonzini@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.