mbox series

[net,0/2] selftests: net: add missing tests to Makefile

Message ID 20220428044511.227416-1-liuhangbin@gmail.com
Headers show
Series selftests: net: add missing tests to Makefile | expand

Message

Hangbin Liu April 28, 2022, 4:45 a.m. UTC 
When generating the selftests to another folder, the fixed tests are
missing as they are not in Makefile. The missing tests are generated
by command:
$ for f in $(ls *.sh); do grep -q $f Makefile || echo $f; done

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?

Hangbin Liu (2):
  selftests/net: add missing tests
  selftests/net/forwarding: add missing tests

 tools/testing/selftests/net/Makefile          |  3 +-
 .../testing/selftests/net/forwarding/Makefile | 33 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

Comments

Hangbin Liu May 17, 2022, 6:23 a.m. UTC | #1 
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. 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?

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
+	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"]
+}
Jakub Kicinski May 17, 2022, 7:45 p.m. UTC | #2 
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"]
> +}
Hangbin Liu May 19, 2022, 3:13 a.m. UTC | #3 
On Wed, May 18, 2022 at 08:25:48AM -0700, Jakub Kicinski wrote:
> On Wed, 18 May 2022 14:01:43 +0800 Hangbin Liu wrote:
> > > > +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
> 
> FWIW this will list just the names of bash scripts with no decoration:
> 
>   git show --pretty="" --name-only -- tools/testing/selftests/*.sh
> 
> And we can get the names of the files with basename:
> 
>   for f in $(git show --pretty="" --name-only); do basename $f; done

This way is easier :)

> > python for testing. So I think there is no need to check all
> > tools/testing/selftests/.*/Makefile. WDYT?
> 
> Not sure I understand, let me explain what I meant in more detail. 
> I think we should make it generic. For example check the Makefile 
> in the same location as the script:
> 
>   grep $(basename $f) $(dirname $f)/Makefile
> 
> And maybe just to be safe one directory level down?
> 
>   grep $(basename $f) $(dirname $(dirname $f))/Makefile
> 
> Instead of hardcoding the expected paths.

Ah, got what you mean. Thanks. I will check how to update the script
and open PR after that.

Cheers
Hangbin