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.