netdev - Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 May 2022 12:45:17 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

On Tue, 17 May 2022 14:23:22 +0800 Hangbin Liu wrote:
> On Fri, Apr 29, 2022 at 05:56:04PM -0700, Jakub Kicinski wrote:
> > On Thu, 28 Apr 2022 12:45:09 +0800 Hangbin Liu wrote:  
> > > I think there need a way to notify the developer when they created a new
> > > file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> > > test robot could help do that?  
> > 
> > Our netdev patch checks are here:
> > 
> > https://github.com/kuba-moo/nipa/tree/master/tests/patch
> > 
> > in case you're willing to code it up and post a PR.  
> 
> Hi Jakub,
> 
> I checked the tools and write a draft patch. But I have a question before post
> the PR.

First off - thanks a log for doing this!

> AFAIK, This bot is only used for checking patches and adding status in
> patchwork. But it doesn't support sending a reply to developer, right?
> 
> For the selftest reminder, I think it would be good to let developer know
> via email if the file is missing in Makefile. What do you think?

Yes, we don't have the auto-reply. There's too much noise in some of
the tests, but mostly it's because we don't want to encourage people
posting patches just to build them. If it's a machine replying rather
than a human some may think that it's okay. We already have
jaw-droppingly expensive VM instance to keep up with the build volume.
And the list is very busy. So we can't afford "post to run the CI"
development model.

> Here is the draft patch:
> 
> diff --git a/tests/patch/check_selftest/check_selftest.sh b/tests/patch/check_selftest/check_selftest.sh
> new file mode 100755
> index 0000000..ad7c608
> --- /dev/null
> +++ b/tests/patch/check_selftest/check_selftest.sh
> @@ -0,0 +1,28 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +rt=0
> +if ! git show --name-status --oneline | \
> +	grep -P '^A\ttools/testing/selftests/net/' | \
> +	grep '\.sh$'; then
> +	echo "No new net selftests script" >&$DESC_FD
> +	exit 0
> +fi
> +
> +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
> +for file in $files; do
> +	if echo $file | grep forwarding; then
> +		file=$(echo $file | sed 's/forwarding\///')
> +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/forwarding/Makefile;then
> +			echo "new test $file not in selftests/net/forwarding/Makefile" >&$DESC_FD
> +			rc=1
> +		fi
> +	else
> +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/Makefile;then
> +			echo "new test $file not in selftests/net/Makefile" >&$DESC_FD
> +			rc=1
> +		fi

Does it matter which exact selftest makefile the changes are?
Maybe as a first stab we should just check if there are changes 
to anything in tools/testing/selftests/.*/Makefile?

We can see if there are false-negatives.

> +	fi
> +done
> +
> +exit $rc
> diff --git a/tests/patch/check_selftest/info.json b/tests/patch/check_selftest/info.json
> new file mode 100644
> index 0000000..615779f
> --- /dev/null
> +++ b/tests/patch/check_selftest/info.json
> @@ -0,0 +1,3 @@
> +{
> +  "run": ["check_selftest.sh"]
> +}