Re: [kvm-unit-tests RFC v2 3/4] run_tests/mkstandalone: add arch dependent function to `for_each_unittest`

[Marc Hartmayer <mhartmay@linux.ibm.com>]

On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote:
>> This allows us, for example, to auto generate a new test case based on
>> an existing test case.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  run_tests.sh            |  2 +-
>>  scripts/common.bash     | 13 +++++++++++++
>>  scripts/mkstandalone.sh |  2 +-
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/run_tests.sh b/run_tests.sh
>> index 24aba9cc3a98..23658392c488 100755
>> --- a/run_tests.sh
>> +++ b/run_tests.sh
>> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT
>>     # preserve stdout so that process_test_output output can write TAP to it
>>     exec 3>&1
>>     test "$tap_output" == "yes" && exec > /dev/null
>> -   for_each_unittest $config run_task
>> +   for_each_unittest $config run_task arch_cmd
>
> Let's just require that arch cmd hook be specified by the "$arch_cmd"
> variable. Then we don't need to pass it to for_each_unittest.

Where is it then specified?

>
>>  ) | postprocess_suite_output
>>  
>>  # wait until all tasks finish
>> diff --git a/scripts/common.bash b/scripts/common.bash
>> index f9c15fd304bd..62931a40b79a 100644
>> --- a/scripts/common.bash
>> +++ b/scripts/common.bash
>> @@ -1,8 +1,15 @@
>> +function arch_cmd()
>> +{
>> +	# Dummy function, can be overwritten by architecture dependent
>> +	# code
>> +	return
>> +}
>
> This dummy function appears unused and can be dropped.

So what is then used if the function is not defined by the architecture
specific code?

>
>>  
>>  function for_each_unittest()
>>  {
>>  	local unittests="$1"
>>  	local cmd="$2"
>> +	local arch_cmd="${3-}"
>>  	local testname
>>  	local smp
>>  	local kernel
>> @@ -19,6 +26,9 @@ function for_each_unittest()
>>  		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
>>  			if [ -n "${testname}" ]; then
>>  				"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +				if [ "${arch_cmd}" ]; then
>> +					"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +				fi
>
> Rather than assuming we should run both $cmd ... and $arch_cmd $cmd ...,
> let's just run $arch_cmd $cmd ..., when it exists. If $arch_cmd wants to
> run $cmd ... first, then it can do so itself.

Yep, makes sense.

>
>>  			fi
>>  			testname=${BASH_REMATCH[1]}
>>  			smp=1
>> @@ -49,6 +59,9 @@ function for_each_unittest()
>>  	done
>>  	if [ -n "${testname}" ]; then
>>  		"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +		if [ "${arch_cmd}" ]; then
>> +			"${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>> +		fi
>>  	fi
>>  	exec {fd}<&-
>>  }
>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> index cefdec30cb33..3b18c0cf090b 100755
>> --- a/scripts/mkstandalone.sh
>> +++ b/scripts/mkstandalone.sh
>> @@ -128,4 +128,4 @@ fi
>>  
>>  mkdir -p tests
>>  
>> -for_each_unittest $cfg mkstandalone
>> +for_each_unittest $cfg mkstandalone arch_cmd
>> -- 
>> 2.25.4
>>
>
> In summary, I think this patch should just be
>
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 9a6ebbd7f287..b409b0529ea6 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -17,7 +17,7 @@ function for_each_unittest()
>  
>         while read -r -u $fd line; do
>                 if [[ "$line" =~ ^\[(.*)\]$ ]]; then
> -                       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +                       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"

If we remove the $arch_cmd variable we can directly use:

arch_cmd "$cmd" …

>                         testname=${BASH_REMATCH[1]}
>                         smp=1
>                         kernel=""
> @@ -45,6 +45,6 @@ function for_each_unittest()
>                         timeout=${BASH_REMATCH[1]}
>                 fi
>         done
> -       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +       "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>         exec {fd}<&-
>  }
>  
>
> Thanks,
> drew
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294