Re: [PATCH 05/28] check: factor out test list building code

[Dave Chinner <david@fromorbit.com>]

On Tue, May 06, 2025 at 05:02:54PM +0530, Nirjhar Roy (IBM) wrote:
> On Thu, 2025-04-17 at 13:00 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Factor out all the test list parsing and building code to
> > common/test_list so that it can be used by both check and
> > check-parallel.
> > 
> > This also namespaces all the test list code to use _tl_ prefixes,
> > and adds wrappers to set up test list parsing parameters.
> > 
> > Note: there is still some future work to convert externally visible
> > parameters like SRC_DIR to the new namespace.
> I have gone through this patch. I did some basic testing too with
> ./check and I havent' found any obvious issues. There are some minor
> feedback that I have given below:

Can you trim the stuff you aren't commenting on out? It makes it
much easier to find what you are commenting on....

....
> > diff --git a/common/test_list b/common/test_list
> > new file mode 100644
> > index 000000000..2432be6f7
> > --- /dev/null
> > +++ b/common/test_list
> > @@ -0,0 +1,295 @@
> > +##/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright (c) 2000-2002,2006 Silicon Graphics, Inc.  All Rights Reserved.
> > +# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
> > +#
> > +# Test list parsing and building functions
> > +#
> > +# Note: this file must stand alone and not be dependent on any other includes,
> We can include common/exit, right? That file doesn't file only has a
> couple of exit related functions (with no executable code) and no
> further dependencies, correct?

I wrote that before common/exit was really a thing.

As it is, I don't think we should be sourcing files from sourced
files. it hides all the dependencies, and results in the same file
being sourced multiple times in the same process context.

i.e. if the top level is doing:

. common/exit
.....
. common/test_list
.....

Then we don't need to source common/exit from common/test_list
because it already has been sourced.

That's what the comment is trying to say, and when I wrote it I was
specifically thinking about the mess that common/rc and
common/config was causing.


> > +# most especially common/rc and common/config. This is because we have to
> > +# include this file before option parsing, whilst the rc/config includes need to
> > +# be included -after- option parsing.
> > +#
> > +# Any function or variable that is public should have a "_tl_" prefix.
> > +
> > +export _tl_src_dir="tests"
> Minor: There was a change[1] which converted some of global variables
> in small case to upper case. Should we follow the same convention here,
> i.e, _TL_SRC_DIR or maybe _tl_SRC_DIR?

I don't think so, I very much dislike the semi-random capitalisation
of namespace-less variables across fstests. I find coe full of upper
case variables much more difficult to read and follow than properly
namespaced lower case variables.

That's the other thing that is important - lots of the global scope
variables sourced from common files are namespaceless and it makes
it easy easy for tests to accidentally step on such variables. Once
the varaibles are namespaced, there is no need for capitalisation
to indicate that it might be a global variable - the namespace
prefix tells you that as well as where it comes from.

> [1] 
> https://web.git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?h=for-next&id=ab459c67c5e0347ab4a5c28eadfe5ee4e3fd2f01

Yeah, that was a case of choosing your battles.

It wasn't a bug, but it was buried in a long series of bug fixes and
at the time people were shouting and being really obnoxious about
the check-parallel changes. There was no point in dying on an
unimportant hill by objecting to that change.

I still do think it was the wrong thing to do because it makes that
code be inconsistent with every other local file loop device
variable. Those devices are not used outside of common/metadump -
it is internal implemenation variable that the code was written
specifically to be hidden from callers wanting to manipulate and
test metadumps...

> > +_SRC_GROUPS="generic"
> > +_GROUP_LIST=
> > +_XGROUP_LIST=
> > +_tl_exact_order=false
> > +_tl_randomise=false
> > +_tl_have_test_args=false
> > +_tl_file="$tmp.test_list"
> > +_tl_exclude_tests=()
> > +_tl_tests=
> Similar comments for all the above lower cased global variables?

I didn't changed the GROUP names because they are internal to the
test list implementation. I was largely moving the code and didn't
want to change anything that was internal that I didn't need to
touch. I'll have a look at cleaning that up.

.....
> > +	# Specified groups to include
> > +	# Note that the CLI processing adds a leading space to the first group
> > +	# parameter, so we have to catch that here checking for "all"
> > +	if ! $_tl_have_test_args && [ "$_GROUP_LIST" == " all" ]; then
> > +		# no test numbers, do everything
> > +		get_all_tests
> > +	else
> > +		for group in $_GROUP_LIST; do
> > +			list=$(get_group_list $group)
> > +			if [ -z "$list" ]; then
> > +				echo "Group \"$group\" is empty or not defined?"
> > +				exit 1
> _fatal "Group \"$group\" is empty or not defined?" ?

Remember that this was posted before those changes were made. This is
the sort of thing that gets fixed on rebase...

> > +_tl_setup_exclude_group()
> > +{
> > +	local xgroup="$1"
> > +
> > +	_XGROUP_LIST="$_XGROUP_LIST ${xgroup//,/ }"
> > +}
> > +
> > +_tl_setup_group()
> > +{
> > +	local group="$1"
> Minor: Since this function is public , do you think it is helpful to
> have a quick sanitization that makes sure that $1 is not empty and
> prints message otherwise? Similar comment for
> _tl_setup_exclude_group(). 
> 
> > +
> > +	_GROUP_LIST="$_GROUP_LIST ${group//,/ }"
> > +}

I don't think it needs it. It should be checked by the caller
if necessary because it is the one doing option parsing to set up
the group.

> > +
> > +_tl_setup_cli()
> Nit: This function mostly sets up test list related variables - so
> maybe a name that suggests that. Maybe something like
> _tl_setup_test_list_vars or _tl_setup_test_vars - just a suggestion, no
> hard preferences here. 

Ok, I'll come up with a new name for it, but keep in mind that this
function is actually parsing arguments that have come directly from
the CLI.

> > +{
> > +	while [ $# -gt 0 ]; do
> > +		case "$1" in
> > +		-*)	echo "Arguments before tests, please!"
> > +			status=1
> > +			exit $status
> use _fatal?
> > +			;;
> > +		*)	# Expand test pattern (e.g. xfs/???, *fs/001)
> > +			local list=$(cd $_tl_src_dir; echo $1)
> > +			local t
> > +
> > +			for t in $list; do
> > +				t=${t#$_tl_src_dir/}
> > +				local test_dir=${t%%/*}
> > +				local test_name=${t##*/}
> > +				local group_file=$_tl_src_dir/$test_dir/group.list
> > +
> > +				if grep -Eq "^$test_name" $group_file; then
> > +					# in group file ... OK
> > +					echo $_tl_src_dir/$test_dir/$test_name \
> > +						>> $_tl_file
> > +					_tl_have_test_args=true
> > +				else
> > +					# oops
> > +					echo "$t - unknown test, ignored"
> Minor: Sometimes it has happened with me that I have added a new test
> (with ./new ) but forgot to run make and hence the test wasn't getting
> run and was reported to be an invalid test. Should we add a suggestion
> to run make, in case we encounter an unidentified test name?

Not in this patch set. If you want these sorts of process reminders,
please send them as separate feature additions.

-Dave.
-- 
Dave Chinner
david@fromorbit.com