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

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

On 5/21/25 09:25, Dave Chinner wrote:
> 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....
Yeah, sorry. I think there was some issue with the editor of my email 
client.
>
> ....
>>> 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.
Yeah, right.
>
> 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.
Okay.
>
>
>>> +# 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
Yeah, accidental re-use and modification of global variable can happen. 
I agree.
> 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...

Okay, got it.

>
>>> +_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.
Okay.
>
> .....
>>> +	# 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...
Yeah, right.
>
>>> +_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.
Okay.
>
>>> +
>>> +_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.
Yes, I have got that.
>>> +{
>>> +	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.

Yeah.

--NR

>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore