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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
