Re: [PATCH 2/2] test: cleanup test scripts

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Mon, 2018-06-11 at 12:45 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Include 'common' file to use some fucntions for test scripts.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  test/blk-exhaust.sh       | 21 +++-------------
>  test/btt-check.sh         | 35 ++++++++------------------
>  test/btt-errors.sh        | 20 ++++-----------
>  test/btt-pad-compat.sh    | 52 ++++++++++++++-------------------------
>  test/clear.sh             | 21 +++-------------
>  test/create.sh            | 21 +++-------------
>  test/daxdev-errors.sh     | 19 +++-----------
>  test/firmware-update.sh   | 25 ++++---------------
>  test/inject-error.sh      | 31 ++++++-----------------
>  test/label-compat.sh      | 21 +++-------------
>  test/multi-dax.sh         | 19 +++-----------
>  test/pmem-errors.sh       | 20 +++++----------
>  test/rescan-partitions.sh | 44 +++++++++------------------------
>  test/sector-mode.sh       |  5 +---
>  14 files changed, 89 insertions(+), 265 deletions(-)

Hi Masayoshi,

This is a welcome cleanup. It looks good except for a couple of nits.
First, can you add a SPDX style license header to the new test/common file?
Second, see below -

[..]

> 
> diff --git a/test/firmware-update.sh b/test/firmware-update.sh
> index c2cf578..1ed60b1 100755
> --- a/test/firmware-update.sh
> +++ b/test/firmware-update.sh
> @@ -12,25 +12,9 @@ rc=77
>  dev=""
>  image="update-fw.img"
>  
> -trap 'err $LINENO' ERR
> +. ./common
>  
> -# $1: Line number
> -# $2: exit code
> -err()
> -{
> -	[ -n "$2" ] && rc="$2"
> -	echo "test/firmware-update.sh: failed at line $1"
> -	exit "$rc"
> -}
> -
> -check_min_kver()
> -{
> -	local ver="$1"
> -	: "${KVER:=$(uname -r)}"
> -
> -	[ -n "$ver" ] || return 1
> -	[[ "$ver" == "$(echo -e "$ver\n$KVER" | sort -V | head -1)" ]]
> -}
> +trap 'err $LINENO' ERR
>  
>  reset()
>  {
> @@ -52,7 +36,7 @@ cleanup()
>  detect()
>  {
>  	dev=$($ndctl list -b "$bus" -D | jq .[0].dev | tr -d '"')
> -	[ -n "$dev" ] || err "$LINENO" 2
> +	[ -n "$dev" ] || rc=2 && err "$LINENO"

This || .. && will not work as expected. If $dev is null it will set rc to
2, and if it exists, then it will err(). What we want is 

[ -n "$dev" ] || { rc=2 && err "$LINENO"; }
Or even better,

   @@ -36,7 +36,7 @@ cleanup()
    detect()
    {
           dev=$($ndctl list -b "$bus" -D | jq .[0].dev | tr -d '"')
   -   [ -n "$dev" ] || rc=2 && err "$LINENO"
   + [ -n "$dev" ] || err "$LINENO"
    }
    
    do_tests()
   @@ -50,6 +50,7 @@ check_min_kver "4.16" || do_skip "may lack firmware update test handling"
    modprobe nfit_test
    rc=1
    reset
   +rc=2
    detect
    do_tests

The rest looks good.

Thanks,
	-Vishal
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm