From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@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
Message-ID: <20160810132205.GA1574@potion>
References: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kvm-ppc@vger.kernel.org,
	lvivier@redhat.com, thuth@redhat.com, drjones@redhat.com
To: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:39592 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752923AbcHJSII (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 10 Aug 2016 14:08:08 -0400
Content-Disposition: inline
In-Reply-To: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.