Re: [PATCH 4/8] fstests: define a common _dump_cleanup function

From: Amir Goldstein <amir73il@gmail.com>
To: Dave Chinner <david@fromorbit.com>
Cc: fstests <fstests@vger.kernel.org>
Subject: Re: [PATCH 4/8] fstests: define a common _dump_cleanup function
Date: Tue, 24 May 2022 12:04:36 +0300	[thread overview]
Message-ID: <CAOQ4uxiKbdMQZsr335RM6Og6XLkSTUdVw7NdsYD3djJbtPGRmQ@mail.gmail.com> (raw)
In-Reply-To: <20220524073411.1943480-5-david@fromorbit.com>

On Tue, May 24, 2022 at 11:32 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Almost all xfsdump tests now use the same cleanup function.
> Deduplicate them.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  common/dump   | 53 ++++++++++++++++++++++++++-------------------------
>  tests/xfs/022 | 12 ++----------
>  tests/xfs/023 | 13 ++-----------
>  tests/xfs/024 | 12 ++----------
>  tests/xfs/025 | 12 ++----------
>  tests/xfs/026 | 12 ++----------
>  tests/xfs/027 | 12 ++----------
>  tests/xfs/028 | 12 ++----------
>  tests/xfs/035 | 10 +---------
>  tests/xfs/036 |  9 +--------
>  tests/xfs/037 | 10 +---------
>  tests/xfs/038 |  9 +--------
>  tests/xfs/039 |  9 +--------
>  tests/xfs/043 |  9 +--------
>  tests/xfs/046 |  9 +--------
>  tests/xfs/047 |  9 +--------
>  tests/xfs/055 |  9 +--------
>  tests/xfs/056 | 11 +----------
>  tests/xfs/059 | 12 ++----------
>  tests/xfs/060 | 12 ++----------
>  tests/xfs/061 | 12 ++----------
>  tests/xfs/063 |  9 ++-------
>  tests/xfs/064 |  9 ++-------
>  tests/xfs/065 |  9 ++-------
>  tests/xfs/066 | 11 +++--------
>  tests/xfs/068 | 12 ++----------
>  tests/xfs/266 | 14 +++-----------
>  tests/xfs/267 | 13 +++----------
>  tests/xfs/268 | 12 +++---------
>  tests/xfs/281 |  9 +--------
>  tests/xfs/282 |  9 +--------
>  tests/xfs/283 |  9 +--------
>  tests/xfs/287 | 11 +++--------
>  tests/xfs/296 | 11 ++---------
>  tests/xfs/301 | 11 ++---------
>  tests/xfs/302 | 11 ++---------
>  tests/xfs/544 | 10 ++--------
>  37 files changed, 91 insertions(+), 347 deletions(-)
>

Nice cleanup!

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

See one question below...
[...]

> diff --git a/tests/xfs/026 b/tests/xfs/026
> index 18529003..0daa7c88 100755
> --- a/tests/xfs/026
> +++ b/tests/xfs/026
> @@ -9,17 +9,8 @@
>  . ./common/preamble
>  _begin_fstest dump ioctl auto quick
>
> -status=0       # success is the default!
> -

All those tests that you change from success is the default
to failure is the default, that makes sense, but
1. Should be documented in the commit message?
2. Why were those tests written this way? Do you know?

Thanks,
Amir.