Re: [PATCH bpf-next v4 1/2] selftests/bpf: do not export subtest as standalone test

From: Hou Tao <houtao1@huawei.com>
To: Yonghong Song <yhs@fb.com>, Hou Tao <hotforest@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>,
	Andrii Nakryiko <andrii@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	<netdev@vger.kernel.org>, <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v4 1/2] selftests/bpf: do not export subtest as standalone test
Date: Tue, 8 Feb 2022 09:36:50 +0800	[thread overview]
Message-ID: <e1300ed8-8118-6463-551d-59f6c5f10086@huawei.com> (raw)
In-Reply-To: <bc45d372-91cf-a909-aa8b-4daa755e758f@fb.com>

Hi,

On 2/8/2022 2:17 AM, Yonghong Song wrote:
>
>
> On 2/5/22 8:31 PM, Hou Tao wrote:
>> Two subtests in ksyms_module.c are not qualified as static, so these
>> subtests are exported as standalone tests in tests.h and lead to
>> confusion for the output of "./test_progs -t ksyms_module".
>>
>> By using the following command:
>>
>>    grep "^void \(serial_\)\?test_[a-zA-Z0-9_]\+(\(void\)\?)" \
>>        tools/testing/selftests/bpf/prog_tests/*.c | \
>>     awk -F : '{print $1}' | sort | uniq -c | awk '$1 != 1'
>>
>> Find out that other tests also have the similar problem, so fix
>> these tests by marking subtests in these tests as static. For
>> xdp_adjust_frags.c, there is just one subtest, so just export
>> the subtest directly.
[...]
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> index 134d0ac32f59..fc2d8fa8dac5 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
>> @@ -102,9 +102,3 @@ void test_xdp_update_frags(void)
>>   out:
>>       bpf_object__close(obj);
>>   }
>> -
>> -void test_xdp_adjust_frags(void)
>> -{
>> -    if (test__start_subtest("xdp_adjust_frags"))
>> -        test_xdp_update_frags();
>> -}
>
> I suggest keep test_xdp_adjust_frags and mark
> test_xdp_update_frags as static function, and
> this is also good for future extension.
> It is confusing that test_xdp_update_frags
> test in file xdp_adjust_frags.c. Typical
> prog_tests/ test has {test,serial_test}_<TEST> test
> with file name <TEST>.c file.
>
Will do and thanks for your suggestion.

Regards,
Tao
> [...]
> .