On Fri, Jun 23, 2023 at 6:29 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> On Wed, Jun 21, 2023 at 03:09:35PM +0300, Amir Goldstein wrote:
> > On Wed, Jun 21, 2023 at 12:24 PM David Disseldorp <ddiss@xxxxxxx> wrote:
> > >
> > > Hi Zorro and Amir,
> > >
> > > On Wed, 21 Jun 2023 10:23:55 +0800, Zorro Lang wrote:
> > >
> > > > On Mon, Jun 19, 2023 at 01:50:58PM +0300, Amir Goldstein wrote:
> > > > > kvm-xfstests invokes check -n twice to pre-process and generate the
> > > > > tests-to-run list, which is then being passed as a long list of tests
> > > > > for invkoing check in the command line.
> > > > >
> > > > > check invokes dirname, basename and sed several times per test just
> > > > > for doing basic string prefix/suffix trimming.
> > > > > Use bash string pattern matching instead which is much faster.
> > > > >
> > > > > Note that the following pattern matching expression change:
> > > > > < test_dir=${test_dir#$SRC_DIR/*}
> > > > > > t=${t#$SRC_DIR/}
> > > > > does not change the meaning of the expression, because the
> > > > > shortest match of "$SRC_DIR/*" that is being trimmed is "$SRC_DIR/"
> > > > > and removing the tests/ prefix is what this code intended to do.
> > > > >
> > > > > With check -n, there is no need to cleanup the results dir,
> > > > > but check -n is doing that for every single listed test.
> > > > > Move the cleanup of results dir to before actually running the test.
> > > > >
> > > > > These improvements to check pre-test code cut down several minutes
> > > > > from the time until tests actually start to run with kvm-xfstests.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Zorro,
> > > > >
> > > > > Just to clarify, this change is not expected to change behavior -
> > > > > only to improve runtime of check -n and the things that happen
> > > > > before the first test is invoked.
> > > > >
> > > > > Thanks,
> > > > > Amir.
> > > > >
> > > > > check | 22 +++++++++-------------
> > > > > 1 file changed, 9 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/check b/check
> > > > > index e36978c1..1a16eccb 100755
> > > > > --- a/check
> > > > > +++ b/check
> > > > > @@ -399,9 +399,9 @@ if $have_test_arg; then
> > > > > *) # Expand test pattern (e.g. xfs/???, *fs/001)
> > > > > list=$(cd $SRC_DIR; echo $1)
> > > > > for t in $list; do
> > > > > - test_dir=`dirname $t`
> > > > > - test_dir=${test_dir#$SRC_DIR/*}
> > > > > - test_name=`basename $t`
> > > > > + t=${t#$SRC_DIR/}
> > > >
> > > > Actually I'm not sure what's this line for. We get $list by "cd $SRC_DIR; echo $1",
> > > > is there any chance to have "$SRC_DIR" in $t (or $test_dir) ?
> > >
> > > Indeed, this line can be dropped. One minor change in behaviour here is
> > > that a test pattern with a wacky path prefix e.g. ../tests/xfs/00? will
> > > no longer work. I think breaking such patterns is okay, but it could
> > > also be resolved by more carefully extracting the last two path
> > > components.
> > >
>
> Hi Amir and David,
>
> Sorry for the late reply, I'm on a holiday of Dragon Boat Festival recently,
> will be back tomorrow :)
>
> >
> > I didn't want to drop it because I didn't want to change legacy behavior.
>
> Sure, I didn't want to remove this line in this patch, due to it's not
> match the target of this patch. Just speak out to check with you and
> others :)
>
> As we agree this line is useless, we can deal with it in later patch
> which improves ./check.
>
> > When user's cwd is fstests root directory, it may be natural for some
> > users to use bash auto completion to run the command
> > ./check tests/generic/001
> >
> > so there may well be users out these that pass test names including
> > the tests/ prefix and check has always trimmed this prefix.
> >
> > There is something a bit confusing about expanding of bash
> > patterns:
> >
> > When the user uses this syntax:
> > ./check tests/generic/00?
>
> I think the "tests/" prefix is not necessary to support :)
>
Fine by me.
> >
> > the shell expands that pattern to a list of 9 arguments passed to
> > ./check tests/generic/001 tests/generic/002 ...
> >
> > But when the user uses the documented syntax:
> > ./check generic/00?
> > check gets a single argument and $(cd $SRC_DIR; echo $1)
> > expands this single argument to a list
> >
> > All the above is existing behavior which my patch should
> > not have changed.
> >
> > > > > + test_dir=${t%/*}
> > > > > + test_name=${t#*/}
> > > >
> >
> > You may change the above to:
> > + test_dir=${t%%/*}
> > + test_name=${t##*/}
>
> OK, this makes more sense to me. I'll give this a test, and merge this
> change if no regression. Or you'd like to send a V2 to change more.
>
I have no plans to send v2 with more changes.
Please make this change on merge.
Thanks,
Amir.
