| Message ID | 1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-08-12 12:00+0200, Andrew Jones: > 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 Ok, experience with `yum` made me tolerant. :) I would go with the check inside scripts/runtime.bash then. > We could also add a couple standard options to standalone tests, > -h (help - output what the test does, warn about crashing hosts, etc.) Sounds nice. Could also work with `./run_tests.sh -h` to print them all. > -y (yes - say yes at any prompts) What about adding a "-g $group" option to standalone tests instead.? We could then use for test in tests/*; do $test -g $group; done to run the same tests as ./run_test.sh -g $group > -h would take its text from the unittests.cfg file (we'd add a new > unit test property called 'help' there) -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 12, 2016 at 02:06:36PM +0200, Radim Krčmář wrote: > 2016-08-12 12:00+0200, Andrew Jones: > > 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 > > Ok, experience with `yum` made me tolerant. :) > I would go with the check inside scripts/runtime.bash then. > > > We could also add a couple standard options to standalone tests, > > -h (help - output what the test does, warn about crashing hosts, etc.) > > Sounds nice. > Could also work with `./run_tests.sh -h` to print them all. Sounds good. > > > -y (yes - say yes at any prompts) > > What about adding a "-g $group" option to standalone tests instead.? I'd rather the concept of group disappear for standalone tests. IMO, a standalone test isn't a member of a group or of a test framework. It's just a script with an embedded binary. > > We could then use > > for test in tests/*; do $test -g $group; done > > to run the same tests as > > ./run_test.sh -g $group Being able to run all standalone tests in a group isn't a bad idea, but to keep the standalone test feel we could provide generated scripts named group-name.sh that does the above. IOW, I'm OK with adding -g support to standalone scripts if it stays hidden within another "just a script" Suraj, IMO, you don't need to worry about these ideas (-h, -y, group-name.sh) for this series. We can do those later. However I'm happy to review anything you pull together along these lines :-) Thanks, drew > > > -h would take its text from the unittests.cfg file (we'd add a new > > unit test property called 'help' there) > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-08-12 at 14:58 +0200, Andrew Jones wrote: > On Fri, Aug 12, 2016 at 02:06:36PM +0200, Radim Krčmář wrote: > > > > 2016-08-12 12:00+0200, Andrew Jones: > > > > > > 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 > > Ok, experience with `yum` made me tolerant. :) > > I would go with the check inside scripts/runtime.bash then. > > > > > > > > We could also add a couple standard options to standalone tests, > > > -h (help - output what the test does, warn about crashing hosts, > > > etc.) > > Sounds nice. > > Could also work with `./run_tests.sh -h` to print them all. > Sounds good. > > > > > > > > > > > -y (yes - say yes at any prompts) > > What about adding a "-g $group" option to standalone tests > > instead.? > I'd rather the concept of group disappear for standalone tests. IMO, > a standalone test isn't a member of a group or of a test framework. > It's just a script with an embedded binary. I agree that it'd be better to keep the idea of groups and standalone tests separate. You shouldn't have to worry about groups when running a standalone test. > > > > > > > We could then use > > > > for test in tests/*; do $test -g $group; done > > > > to run the same tests as > > > > ./run_test.sh -g $group > Being able to run all standalone tests in a group isn't a bad idea, > but to keep the standalone test feel we could provide generated > scripts > named group-name.sh that does the above. IOW, I'm OK with adding -g > support to standalone scripts if it stays hidden within another > "just a script" The idea of being able to run all tests in a given group makes sense. Although we could see the case with some overlap if a test subscribes to many different groups, although currently this isn't the case with any existing tests and may never be the case. > > Suraj, > > IMO, you don't need to worry about these ideas (-h, -y, group- > name.sh) > for this series. We can do those later. However I'm happy to review > anything you pull together along these lines :-) Alright, for this series I think I'll move the checking into a function in scripts/runtime.bash and call it a day. Then I'll look at putting some of the -h and -y functionality into another series. > > Thanks, > drew > > > > > > > > > > > -h would take its text from the unittests.cfg file (we'd add a > > > new > > > unit test property called 'help' there) > > -- > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" 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..64e85c9 100755 --- 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" + echo "standalone=\"true\"" + fi + echo "run ${args[@]}" } diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 0503cf0..a7a2250 100644 --- 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 + 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.
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> --- arm/unittests.cfg | 3 +++ powerpc/unittests.cfg | 3 +++ scripts/mkstandalone.sh | 21 +++++++++++++++++++++ scripts/runtime.bash | 8 +++++++- x86/unittests.cfg | 3 +++ 5 files changed, 37 insertions(+), 1 deletion(-)