Re: [PATCH mptcp-next v5 07/12] selftests: mptcp: print test results with counters

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 28/02/2024 09:23, Geliang Tang wrote:
> On Mon, 2024-02-26 at 13:41 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 26/02/2024 10:43, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch adds a new helper mptcp_lib_print_test_counter() to
>>> print
>>> out test counter in each test result and increase the counter. Use
>>> this helper to print out test counters for every tests in diag.sh,
>>> mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
>>> and userspace_pm.sh.

(...)

>>> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
>>> b/tools/testing/selftests/net/mptcp/diag.sh
>>> index f9f62a8f41e3..01e9f11f1f47 100755
>>> --- a/tools/testing/selftests/net/mptcp/diag.sh
>>> +++ b/tools/testing/selftests/net/mptcp/diag.sh
>>> @@ -9,7 +9,7 @@
>>>  . "$(dirname "${0}")/mptcp_lib.sh"
>>>  
>>>  ns=""
>>> -test_cnt=1
>>> +test_cnt=0
>>
>> Is it normal shellcheck doesn't complain about it not being used?
> 
> test_cnt still be used in this script in another place:
> 
> 	ret=$test_cnt

Oh, I remember I saw that when fixing other thing and apparently I
forgot about that: we should stop doing that! e.g. what if only the 4th
test fail? → We will do 'exit 4' which is 'exit ${KSFT_SKIP}' → the
whole test will be marked as skipped instead of 'failed'!

So we should do ret=${KSFT_FAIL} (or ret=1).

Do you mind sending a patch fixing that for mptcp-net please? (with a
Fixes tag: I guess it is there from the beginning or almost).

>>>  timeout_poll=30
>>>  timeout_test=$((timeout_poll * 2 + 1))
>>>  ret=0

(...)

>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> index 7e309493eda2..df495658f043 100644
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> @@ -411,3 +411,11 @@ mptcp_lib_events() {
>>>  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1
>>> &
>>>  	pid=$!
>>>  }
>>> +
>>> +mptcp_lib_print_test_counter() {
>>
>> Maybe more "mptcp_lib_print_test_title" or "_name": it doesn't just
>> print the counter, mostly the "title" (including the counter, part of
>> the title)
> 
> Rename to mptcp_lib_print_title.
> 
>>
>>> +	declare -n counter="${1}"
>>> +	local fmt="${2}"
>>> +	local msg="${3}"
>>> +
>>> +	printf "%02u ${fmt}" "$((++counter))" "${msg}"
>>
>> Maybe having this:
>>
>>   : "${MPTCP_LIB_PRINT_TEST_FORMAT:="%02u %-50s"}"
>>   MPTCP_LIB_TEST_COUNTER=0
>>
>>   (...)
>>
>>   # $1: test name
>>   mptcp_lib_print_test_counter() {
>>       printf "${MPTCP_LIB_PRINT_TEST_FORMAT}" \
>>              "$((++MPTCP_LIB_TEST_COUNTER))" "${1}"
>>   }
>>
>> Would simplify stuff?
> 
> Do you mean to define TEST_COUNT in mptcp_lib.sh? That means we need to
> rename test_cnt to TEST_COUNT. I remember you objected to move vars
> into mptcp_lib.sh like in "selftests: mptcp: add mptcp_lib_ns_init/exit
> helpers". So I pass TEST_COUNT or test_cnt as an argument to
> mptcp_lib_print_test_counter().

Here it is different I think: here I suggest moving TEST_COUNT to
mptcp_lib.sh (renamed MPTCP_LIB_TEST_COUNTER), use it for all tests, and
only use it in mptcp_lib.sh. For the netns vars, they were use in many
places, with different names, and not the same number → we didn't want
to rename them everywhere. Here, it is a basic counter, just a single
variable that can be used in all tests, in very specific places (only to
read it, not to modify it).

I don't know, it looks different to me. If we can use it the same way
everywhere (when printing test title), that looks like a good modification.

> I think this can keep continuity with other functions in mptcp_lib,
> like mptcp_lib_ns_init/exit.
> 
>>
>> So tests can specify the format by setting
>> MPTCP_LIB_PRINT_TEST_FORMAT,
>> e.g. to support more than 99 entries. And you don't need to define
>> TEST_COUNT and disable SC2034.
>>
>> Would that work?
>>
>> You could even use it in mptcp_join.sh: print_title() would call this
>> new helper.
>>
>>> +}
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.