[PATCH 0/2] add checks for tests not in a suite

[PATCH 0/2] add checks for tests not in a suite

[Bruce Richardson]

To help ensure that we don't have "orphaned" tests not in any test
suites we can add the following checks:

* In developer-mode builds, emit a warning for each test defined using
  REGISTER_TEST_COMMAND
* In checkpatches, add a check to prevent the addition of new tests
  using the REGISTER_TEST_COMMAND macro

Bruce Richardson (2):
  app/test: emit warning for tests not in a test suite
  devtools: check for tests added without a test suite

 app/test/suites/meson.build   | 13 ++++++++++++-
 buildtools/get-test-suites.py | 12 +++++++++---
 devtools/checkpatches.sh      |  8 ++++++++
 3 files changed, 29 insertions(+), 4 deletions(-)

--
2.39.2

[PATCH 1/2] app/test: emit warning for tests not in a test suite

[Bruce Richardson]

When doing a scan for tests to add to test suites, we can also detect
the tests which have not been added to any suite. By doing so:

a) we can emit a warning informing the developer that the test is not in
   a suite (if developer mode is enabled)
b) we can still register a test for that command making these tests
   callable through "meson test"

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/suites/meson.build   | 13 ++++++++++++-
 buildtools/get-test-suites.py | 12 +++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/app/test/suites/meson.build b/app/test/suites/meson.build
index 19a6b902fa..478f245a54 100644
--- a/app/test/suites/meson.build
+++ b/app/test/suites/meson.build
@@ -21,7 +21,18 @@ foreach suite:test_suites
     suite = suite.split('=')
     suite_name = suite[0]
     suite_tests = suite[1].split(',')
-    if suite_name != 'fast-tests'
+    if suite_name == 'non_suite_tests'
+        # tests not in any suite
+        foreach t: suite_tests
+            if developer_mode
+                warning('Test "@0@" is not defined in any test suite'.format(t))
+            endif
+            test(t, dpdk_test,
+                    env: ['DPDK_TEST=' + t],
+                    timeout: timeout_seconds,
+                    is_parallel: false)
+        endforeach
+    elif suite_name != 'fast-tests'
         # simple cases - tests without parameters or special handling
         foreach t: suite_tests
             test(t, dpdk_test,
diff --git a/buildtools/get-test-suites.py b/buildtools/get-test-suites.py
index 95a9cad4c8..574c233aa8 100644
--- a/buildtools/get-test-suites.py
+++ b/buildtools/get-test-suites.py
@@ -8,18 +8,23 @@
 input_list = sys.argv[1:]
 test_def_regex = re.compile("REGISTER_([A-Z]+)_TEST\s*\(\s*([a-z0-9_]+)")
 test_suites = {}
+# track tests not in any test suite.
+non_suite_regex = re.compile("REGISTER_TEST_COMMAND\s*\(\s*([a-z0-9_]+)")
+non_suite_tests = []
 
 def get_fast_test_params(test_name, ln):
     "Extract the extra fast-test parameters from the line"
-    #print(f"ln: {ln.rstrip()}, test_name: {test_name}, split: {ln.split(test_name, 1)}")
     (_, rest_of_line) = ln.split(test_name, 1)
     (_, nohuge, asan, _func) = rest_of_line.split(',', 3)
     return f":{nohuge.strip().lower()}:{asan.strip().lower()}"
 
 for fname in input_list:
     with open(fname) as f:
-        contents = [ln for ln in f.readlines() if test_def_regex.match(ln.strip())]
-    for ln in contents:
+        contents = [ln.strip() for ln in f.readlines()]
+        test_lines = [ln for ln in contents if test_def_regex.match(ln)]
+        non_suite_tests.extend([non_suite_regex.match(ln).group(1)
+                for ln in contents if non_suite_regex.match(ln)])
+    for ln in test_lines:
         (test_suite, test_name) = test_def_regex.match(ln).group(1, 2)
         suite_name = f"{test_suite.lower()}-tests"
         if suite_name in test_suites:
@@ -31,3 +36,4 @@ def get_fast_test_params(test_name, ln):
 
 for suite in test_suites.keys():
     print(f"{suite}={','.join(test_suites[suite])}")
+print(f"non_suite_tests={','.join(non_suite_tests)}")
-- 
2.39.2

[PATCH 2/2] devtools: check for tests added without a test suite

[Bruce Richardson]

Add a check in checkpatches.sh for the use of the old
REGISTER_TEST_COMMAND macro, which just adds a test without including it
in a test suite. Suggest to the user to add the test using the newer
macros which will include the test in a test suite.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 devtools/checkpatches.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 55fabc5458..9b201db0a2 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -159,6 +159,14 @@ check_forbidden_additions() { # <patch>
 		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
 		"$1" || res=1
 
+	# prevent addition of tests not in one of our test suites
+	awk -v FOLDERS='app/test' \
+		-v EXPRESSIONS='REGISTER_TEST_COMMAND' \
+		-v RET_ON_FAIL=1 \
+		-v MESSAGE='Using REGISTER_TEST_COMMAND instead of REGISTER_<suite_name>_TEST' \
+		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+		"$1" || res=1
+
 	# SVG must be included with wildcard extension to allow conversion
 	awk -v FOLDERS='doc' \
 		-v EXPRESSIONS='::[[:space:]]*[^[:space:]]*\\.svg' \
-- 
2.39.2

Re: [PATCH 0/2] add checks for tests not in a suite

[Bruce Richardson]

On Fri, Sep 15, 2023 at 12:52:04PM +0100, Bruce Richardson wrote:
> To help ensure that we don't have "orphaned" tests not in any test
> suites we can add the following checks:
> 
> * In developer-mode builds, emit a warning for each test defined using
>   REGISTER_TEST_COMMAND
> * In checkpatches, add a check to prevent the addition of new tests
>   using the REGISTER_TEST_COMMAND macro
> 
> Bruce Richardson (2):
>   app/test: emit warning for tests not in a test suite
>   devtools: check for tests added without a test suite
> 
While not required, if testing this set, it's recommended to apply patch
[1] from David first, to reduce the warning count visible.

/Bruce

[1] http://patches.dpdk.org/project/dpdk/patch/20230915075801.288931-1-david.marchand@redhat.com/

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> To help ensure that we don't have "orphaned" tests not in any test
> suites we can add the following checks:
>
> * In developer-mode builds, emit a warning for each test defined using
>   REGISTER_TEST_COMMAND
> * In checkpatches, add a check to prevent the addition of new tests
>   using the REGISTER_TEST_COMMAND macro
>
> Bruce Richardson (2):
>   app/test: emit warning for tests not in a test suite
>   devtools: check for tests added without a test suite
>
>  app/test/suites/meson.build   | 13 ++++++++++++-
>  buildtools/get-test-suites.py | 12 +++++++++---
>  devtools/checkpatches.sh      |  8 ++++++++
>  3 files changed, 29 insertions(+), 4 deletions(-)

The "non_suite_tests" testsuite returned by
buildtools/get-test-suites.py is a bit strange, as it is not a
testsuite from meson pov.
But otherwise, the series looks good to me.


-- 
David Marchand

Re: [PATCH 0/2] add checks for tests not in a suite

[Bruce Richardson]

On Tue, Sep 19, 2023 at 10:29:07AM +0200, David Marchand wrote:
> On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > To help ensure that we don't have "orphaned" tests not in any test
> > suites we can add the following checks:
> >
> > * In developer-mode builds, emit a warning for each test defined using
> >   REGISTER_TEST_COMMAND
> > * In checkpatches, add a check to prevent the addition of new tests
> >   using the REGISTER_TEST_COMMAND macro
> >
> > Bruce Richardson (2):
> >   app/test: emit warning for tests not in a test suite
> >   devtools: check for tests added without a test suite
> >
> >  app/test/suites/meson.build   | 13 ++++++++++++-
> >  buildtools/get-test-suites.py | 12 +++++++++---
> >  devtools/checkpatches.sh      |  8 ++++++++
> >  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> The "non_suite_tests" testsuite returned by
> buildtools/get-test-suites.py is a bit strange, as it is not a
> testsuite from meson pov.

Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
did it that way to avoid having yet another script to scan the files - I
figured it was faster (in terms of runtime, not dev time) to do the
scanning when the files are already being opened and processed by this one.

Of course, if we can get the un-suitened [:-)] test cases down to zero, we
can theoretically drop this check in future, and  just use the checkpatch
one.

/Bruce

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Sep 19, 2023 at 10:29:07AM +0200, David Marchand wrote:
> > On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > To help ensure that we don't have "orphaned" tests not in any test
> > > suites we can add the following checks:
> > >
> > > * In developer-mode builds, emit a warning for each test defined using
> > >   REGISTER_TEST_COMMAND
> > > * In checkpatches, add a check to prevent the addition of new tests
> > >   using the REGISTER_TEST_COMMAND macro
> > >
> > > Bruce Richardson (2):
> > >   app/test: emit warning for tests not in a test suite
> > >   devtools: check for tests added without a test suite
> > >
> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> > >  buildtools/get-test-suites.py | 12 +++++++++---
> > >  devtools/checkpatches.sh      |  8 ++++++++
> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> >
> > The "non_suite_tests" testsuite returned by
> > buildtools/get-test-suites.py is a bit strange, as it is not a
> > testsuite from meson pov.
>
> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> did it that way to avoid having yet another script to scan the files - I
> figured it was faster (in terms of runtime, not dev time) to do the

I had figured it was "faster dev time" that won :-).
I am fine with it, I don't expect more complications in this area in the future.


> scanning when the files are already being opened and processed by this one.
>
> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> can theoretically drop this check in future, and  just use the checkpatch
> one.

Well, that's still a question that nobody seems to comment on.

What should we do with tests that don't enter one of those testsuites,
and are not invoked by the CI?

Though we may be removing some level of coverage, I am for
cleaning/unused dead code.


-- 
David Marchand

Re: [PATCH 0/2] add checks for tests not in a suite

[Aaron Conole]

David Marchand <david.marchand@redhat.com> writes:

> On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>>
>> On Tue, Sep 19, 2023 at 10:29:07AM +0200, David Marchand wrote:
>> > On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson
>> > <bruce.richardson@intel.com> wrote:
>> > >
>> > > To help ensure that we don't have "orphaned" tests not in any test
>> > > suites we can add the following checks:
>> > >
>> > > * In developer-mode builds, emit a warning for each test defined using
>> > >   REGISTER_TEST_COMMAND
>> > > * In checkpatches, add a check to prevent the addition of new tests
>> > >   using the REGISTER_TEST_COMMAND macro
>> > >
>> > > Bruce Richardson (2):
>> > >   app/test: emit warning for tests not in a test suite
>> > >   devtools: check for tests added without a test suite
>> > >
>> > >  app/test/suites/meson.build   | 13 ++++++++++++-
>> > >  buildtools/get-test-suites.py | 12 +++++++++---
>> > >  devtools/checkpatches.sh      |  8 ++++++++
>> > >  3 files changed, 29 insertions(+), 4 deletions(-)
>> >
>> > The "non_suite_tests" testsuite returned by
>> > buildtools/get-test-suites.py is a bit strange, as it is not a
>> > testsuite from meson pov.
>>
>> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
>> did it that way to avoid having yet another script to scan the files - I
>> figured it was faster (in terms of runtime, not dev time) to do the
>
> I had figured it was "faster dev time" that won :-).
> I am fine with it, I don't expect more complications in this area in the future.
>
>
>> scanning when the files are already being opened and processed by this one.
>>
>> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
>> can theoretically drop this check in future, and  just use the checkpatch
>> one.
>
> Well, that's still a question that nobody seems to comment on.
>
> What should we do with tests that don't enter one of those testsuites,
> and are not invoked by the CI?
>
> Though we may be removing some level of coverage, I am for
> cleaning/unused dead code.

I guess it does require actually looking at these tests and classifying
them to either put them into the proper suites.  As of now, we aren't
really removing coverage if they aren't being run - but are any
maintainers or developers actually running them?

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
> David Marchand <david.marchand@redhat.com> writes:
> > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> >> > > To help ensure that we don't have "orphaned" tests not in any test
> >> > > suites we can add the following checks:
> >> > >
> >> > > * In developer-mode builds, emit a warning for each test defined using
> >> > >   REGISTER_TEST_COMMAND
> >> > > * In checkpatches, add a check to prevent the addition of new tests
> >> > >   using the REGISTER_TEST_COMMAND macro
> >> > >
> >> > > Bruce Richardson (2):
> >> > >   app/test: emit warning for tests not in a test suite
> >> > >   devtools: check for tests added without a test suite
> >> > >
> >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> >> > >  buildtools/get-test-suites.py | 12 +++++++++---
> >> > >  devtools/checkpatches.sh      |  8 ++++++++
> >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> >> >
> >> > The "non_suite_tests" testsuite returned by
> >> > buildtools/get-test-suites.py is a bit strange, as it is not a
> >> > testsuite from meson pov.
> >>
> >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> >> did it that way to avoid having yet another script to scan the files - I
> >> figured it was faster (in terms of runtime, not dev time) to do the
> >
> > I had figured it was "faster dev time" that won :-).
> > I am fine with it, I don't expect more complications in this area in the future.
> >
> >
> >> scanning when the files are already being opened and processed by this one.
> >>
> >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> >> can theoretically drop this check in future, and  just use the checkpatch
> >> one.
> >
> > Well, that's still a question that nobody seems to comment on.
> >
> > What should we do with tests that don't enter one of those testsuites,
> > and are not invoked by the CI?
> >
> > Though we may be removing some level of coverage, I am for
> > cleaning/unused dead code.
>
> I guess it does require actually looking at these tests and classifying
> them to either put them into the proper suites.  As of now, we aren't
> really removing coverage if they aren't being run - but are any
> maintainers or developers actually running them?

Could we go a step further than Bruce runtime warning (which is at the
meson level and does not impact running the test)?
Perhaps have those orphaned tests fail unless their test names are
provided in a env variable like
DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
;-))?

With a systematic failure, there is less chance that
developers/maintainers miss the situation.
If those developers/maintainers simply waive the warning with the env
variable and don't send a patch, well.. too bad.

After a release or two, if we don't hear from anyone, we can start
removing the unused one.


-- 
David Marchand

Re: [PATCH 0/2] add checks for tests not in a suite

[Bruce Richardson]

On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote:
> On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
> > David Marchand <david.marchand@redhat.com> writes:
> > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > >> > > To help ensure that we don't have "orphaned" tests not in any test
> > >> > > suites we can add the following checks:
> > >> > >
> > >> > > * In developer-mode builds, emit a warning for each test defined using
> > >> > >   REGISTER_TEST_COMMAND
> > >> > > * In checkpatches, add a check to prevent the addition of new tests
> > >> > >   using the REGISTER_TEST_COMMAND macro
> > >> > >
> > >> > > Bruce Richardson (2):
> > >> > >   app/test: emit warning for tests not in a test suite
> > >> > >   devtools: check for tests added without a test suite
> > >> > >
> > >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> > >> > >  buildtools/get-test-suites.py | 12 +++++++++---
> > >> > >  devtools/checkpatches.sh      |  8 ++++++++
> > >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> > >> >
> > >> > The "non_suite_tests" testsuite returned by
> > >> > buildtools/get-test-suites.py is a bit strange, as it is not a
> > >> > testsuite from meson pov.
> > >>
> > >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> > >> did it that way to avoid having yet another script to scan the files - I
> > >> figured it was faster (in terms of runtime, not dev time) to do the
> > >
> > > I had figured it was "faster dev time" that won :-).
> > > I am fine with it, I don't expect more complications in this area in the future.
> > >
> > >
> > >> scanning when the files are already being opened and processed by this one.
> > >>
> > >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> > >> can theoretically drop this check in future, and  just use the checkpatch
> > >> one.
> > >
> > > Well, that's still a question that nobody seems to comment on.
> > >
> > > What should we do with tests that don't enter one of those testsuites,
> > > and are not invoked by the CI?
> > >
> > > Though we may be removing some level of coverage, I am for
> > > cleaning/unused dead code.
> >
> > I guess it does require actually looking at these tests and classifying
> > them to either put them into the proper suites.  As of now, we aren't
> > really removing coverage if they aren't being run - but are any
> > maintainers or developers actually running them?
> 
> Could we go a step further than Bruce runtime warning (which is at the
> meson level and does not impact running the test)?
> Perhaps have those orphaned tests fail unless their test names are
> provided in a env variable like
> DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
> ;-))?
> 
> With a systematic failure, there is less chance that
> developers/maintainers miss the situation.
> If those developers/maintainers simply waive the warning with the env
> variable and don't send a patch, well.. too bad.
> 
> After a release or two, if we don't hear from anyone, we can start
> removing the unused one.
> 
I think that seems a littel severe at this point.

The one gap we have right now, as far as I can see, is actually explaining
what the various suite types are, so that developers can choose the right
one for their test(s).  We may even need a couple more if some tests do not
fit into the existing categories. Once that is done, we should then start
looking for tests that are obsolete and can be removed.

If we do already have documentation on the various suites and how to use
them, apologies for my ignorance, and perhaps someone could post a link
here.

/Bruce

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Wed, Sep 27, 2023 at 10:27 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote:
> > On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
> > > David Marchand <david.marchand@redhat.com> writes:
> > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > >> > > To help ensure that we don't have "orphaned" tests not in any test
> > > >> > > suites we can add the following checks:
> > > >> > >
> > > >> > > * In developer-mode builds, emit a warning for each test defined using
> > > >> > >   REGISTER_TEST_COMMAND
> > > >> > > * In checkpatches, add a check to prevent the addition of new tests
> > > >> > >   using the REGISTER_TEST_COMMAND macro
> > > >> > >
> > > >> > > Bruce Richardson (2):
> > > >> > >   app/test: emit warning for tests not in a test suite
> > > >> > >   devtools: check for tests added without a test suite
> > > >> > >
> > > >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> > > >> > >  buildtools/get-test-suites.py | 12 +++++++++---
> > > >> > >  devtools/checkpatches.sh      |  8 ++++++++
> > > >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> > > >> >
> > > >> > The "non_suite_tests" testsuite returned by
> > > >> > buildtools/get-test-suites.py is a bit strange, as it is not a
> > > >> > testsuite from meson pov.
> > > >>
> > > >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> > > >> did it that way to avoid having yet another script to scan the files - I
> > > >> figured it was faster (in terms of runtime, not dev time) to do the
> > > >
> > > > I had figured it was "faster dev time" that won :-).
> > > > I am fine with it, I don't expect more complications in this area in the future.
> > > >
> > > >
> > > >> scanning when the files are already being opened and processed by this one.
> > > >>
> > > >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> > > >> can theoretically drop this check in future, and  just use the checkpatch
> > > >> one.
> > > >
> > > > Well, that's still a question that nobody seems to comment on.
> > > >
> > > > What should we do with tests that don't enter one of those testsuites,
> > > > and are not invoked by the CI?
> > > >
> > > > Though we may be removing some level of coverage, I am for
> > > > cleaning/unused dead code.
> > >
> > > I guess it does require actually looking at these tests and classifying
> > > them to either put them into the proper suites.  As of now, we aren't
> > > really removing coverage if they aren't being run - but are any
> > > maintainers or developers actually running them?
> >
> > Could we go a step further than Bruce runtime warning (which is at the
> > meson level and does not impact running the test)?
> > Perhaps have those orphaned tests fail unless their test names are
> > provided in a env variable like
> > DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
> > ;-))?
> >
> > With a systematic failure, there is less chance that
> > developers/maintainers miss the situation.
> > If those developers/maintainers simply waive the warning with the env
> > variable and don't send a patch, well.. too bad.
> >
> > After a release or two, if we don't hear from anyone, we can start
> > removing the unused one.
> >
> I think that seems a littel severe at this point.
>
> The one gap we have right now, as far as I can see, is actually explaining
> what the various suite types are, so that developers can choose the right
> one for their test(s).  We may even need a couple more if some tests do not
> fit into the existing categories. Once that is done, we should then start
> looking for tests that are obsolete and can be removed.
>
> If we do already have documentation on the various suites and how to use
> them, apologies for my ignorance, and perhaps someone could post a link
> here.

We have some guidelines but I agree the testsuites are not described.
https://doc.dpdk.org/guides/contributing/unit_test.html


-- 
David Marchand

Re: [PATCH 0/2] add checks for tests not in a suite

[Bruce Richardson]

On Wed, Sep 27, 2023 at 11:31:07AM +0200, David Marchand wrote:
> On Wed, Sep 27, 2023 at 10:27 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote:
> > > On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
> > > > David Marchand <david.marchand@redhat.com> writes:
> > > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > >> > > To help ensure that we don't have "orphaned" tests not in any test
> > > > >> > > suites we can add the following checks:
> > > > >> > >
> > > > >> > > * In developer-mode builds, emit a warning for each test defined using
> > > > >> > >   REGISTER_TEST_COMMAND
> > > > >> > > * In checkpatches, add a check to prevent the addition of new tests
> > > > >> > >   using the REGISTER_TEST_COMMAND macro
> > > > >> > >
> > > > >> > > Bruce Richardson (2):
> > > > >> > >   app/test: emit warning for tests not in a test suite
> > > > >> > >   devtools: check for tests added without a test suite
> > > > >> > >
> > > > >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> > > > >> > >  buildtools/get-test-suites.py | 12 +++++++++---
> > > > >> > >  devtools/checkpatches.sh      |  8 ++++++++
> > > > >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> > > > >> >
> > > > >> > The "non_suite_tests" testsuite returned by
> > > > >> > buildtools/get-test-suites.py is a bit strange, as it is not a
> > > > >> > testsuite from meson pov.
> > > > >>
> > > > >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> > > > >> did it that way to avoid having yet another script to scan the files - I
> > > > >> figured it was faster (in terms of runtime, not dev time) to do the
> > > > >
> > > > > I had figured it was "faster dev time" that won :-).
> > > > > I am fine with it, I don't expect more complications in this area in the future.
> > > > >
> > > > >
> > > > >> scanning when the files are already being opened and processed by this one.
> > > > >>
> > > > >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> > > > >> can theoretically drop this check in future, and  just use the checkpatch
> > > > >> one.
> > > > >
> > > > > Well, that's still a question that nobody seems to comment on.
> > > > >
> > > > > What should we do with tests that don't enter one of those testsuites,
> > > > > and are not invoked by the CI?
> > > > >
> > > > > Though we may be removing some level of coverage, I am for
> > > > > cleaning/unused dead code.
> > > >
> > > > I guess it does require actually looking at these tests and classifying
> > > > them to either put them into the proper suites.  As of now, we aren't
> > > > really removing coverage if they aren't being run - but are any
> > > > maintainers or developers actually running them?
> > >
> > > Could we go a step further than Bruce runtime warning (which is at the
> > > meson level and does not impact running the test)?
> > > Perhaps have those orphaned tests fail unless their test names are
> > > provided in a env variable like
> > > DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
> > > ;-))?
> > >
> > > With a systematic failure, there is less chance that
> > > developers/maintainers miss the situation.
> > > If those developers/maintainers simply waive the warning with the env
> > > variable and don't send a patch, well.. too bad.
> > >
> > > After a release or two, if we don't hear from anyone, we can start
> > > removing the unused one.
> > >
> > I think that seems a littel severe at this point.
> >
> > The one gap we have right now, as far as I can see, is actually explaining
> > what the various suite types are, so that developers can choose the right
> > one for their test(s).  We may even need a couple more if some tests do not
> > fit into the existing categories. Once that is done, we should then start
> > looking for tests that are obsolete and can be removed.
> >
> > If we do already have documentation on the various suites and how to use
> > them, apologies for my ignorance, and perhaps someone could post a link
> > here.
> 
> We have some guidelines but I agree the testsuites are not described.
> https://doc.dpdk.org/guides/contributing/unit_test.html
> 
I also see I missed updating the reference to REGISTER_TEST_COMMAND in the
docs. I'll do a patch for a doc update for that.

/Bruce

Re: [PATCH 0/2] add checks for tests not in a suite

[Aaron Conole]

Bruce Richardson <bruce.richardson@intel.com> writes:

> On Wed, Sep 27, 2023 at 11:31:07AM +0200, David Marchand wrote:
>> On Wed, Sep 27, 2023 at 10:27 AM Bruce Richardson
>> <bruce.richardson@intel.com> wrote:
>> > On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote:
>> > > On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
>> > > > David Marchand <david.marchand@redhat.com> writes:
>> > > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
>> > > > > <bruce.richardson@intel.com> wrote:
>> > > > >> > > To help ensure that we don't have "orphaned" tests not in any test
>> > > > >> > > suites we can add the following checks:
>> > > > >> > >
>> > > > >> > > * In developer-mode builds, emit a warning for each test defined using
>> > > > >> > >   REGISTER_TEST_COMMAND
>> > > > >> > > * In checkpatches, add a check to prevent the addition of new tests
>> > > > >> > >   using the REGISTER_TEST_COMMAND macro
>> > > > >> > >
>> > > > >> > > Bruce Richardson (2):
>> > > > >> > >   app/test: emit warning for tests not in a test suite
>> > > > >> > >   devtools: check for tests added without a test suite
>> > > > >> > >
>> > > > >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
>> > > > >> > >  buildtools/get-test-suites.py | 12 +++++++++---
>> > > > >> > >  devtools/checkpatches.sh      |  8 ++++++++
>> > > > >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
>> > > > >> >
>> > > > >> > The "non_suite_tests" testsuite returned by
>> > > > >> > buildtools/get-test-suites.py is a bit strange, as it is not a
>> > > > >> > testsuite from meson pov.
>> > > > >>
>> > > > >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
>> > > > >> did it that way to avoid having yet another script to scan the files - I
>> > > > >> figured it was faster (in terms of runtime, not dev time) to do the
>> > > > >
>> > > > > I had figured it was "faster dev time" that won :-).
>> > > > > I am fine with it, I don't expect more complications in this area in the future.
>> > > > >
>> > > > >
>> > > > >> scanning when the files are already being opened and processed by this one.
>> > > > >>
>> > > > >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
>> > > > >> can theoretically drop this check in future, and  just use the checkpatch
>> > > > >> one.
>> > > > >
>> > > > > Well, that's still a question that nobody seems to comment on.
>> > > > >
>> > > > > What should we do with tests that don't enter one of those testsuites,
>> > > > > and are not invoked by the CI?
>> > > > >
>> > > > > Though we may be removing some level of coverage, I am for
>> > > > > cleaning/unused dead code.
>> > > >
>> > > > I guess it does require actually looking at these tests and classifying
>> > > > them to either put them into the proper suites.  As of now, we aren't
>> > > > really removing coverage if they aren't being run - but are any
>> > > > maintainers or developers actually running them?
>> > >
>> > > Could we go a step further than Bruce runtime warning (which is at the
>> > > meson level and does not impact running the test)?
>> > > Perhaps have those orphaned tests fail unless their test names are
>> > > provided in a env variable like
>> > > DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
>> > > ;-))?
>> > >
>> > > With a systematic failure, there is less chance that
>> > > developers/maintainers miss the situation.
>> > > If those developers/maintainers simply waive the warning with the env
>> > > variable and don't send a patch, well.. too bad.
>> > >
>> > > After a release or two, if we don't hear from anyone, we can start
>> > > removing the unused one.
>> > >
>> > I think that seems a littel severe at this point.
>> >
>> > The one gap we have right now, as far as I can see, is actually explaining
>> > what the various suite types are, so that developers can choose the right
>> > one for their test(s).  We may even need a couple more if some tests do not
>> > fit into the existing categories. Once that is done, we should then start
>> > looking for tests that are obsolete and can be removed.
>> >
>> > If we do already have documentation on the various suites and how to use
>> > them, apologies for my ignorance, and perhaps someone could post a link
>> > here.
>> 
>> We have some guidelines but I agree the testsuites are not described.
>> https://doc.dpdk.org/guides/contributing/unit_test.html
>> 
> I also see I missed updating the reference to REGISTER_TEST_COMMAND in the
> docs. I'll do a patch for a doc update for that.

Yeah, we should do some kind of breakout definition of the test suites.
I will try to cook a patch, but the suites themselves are a bit
ambiguous at the moment.  Gotta start somewhere, though.

> /Bruce

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Wed, Sep 27, 2023 at 10:27 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote:
> > On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole <aconole@redhat.com> wrote:
> > > David Marchand <david.marchand@redhat.com> writes:
> > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > >> > > To help ensure that we don't have "orphaned" tests not in any test
> > > >> > > suites we can add the following checks:
> > > >> > >
> > > >> > > * In developer-mode builds, emit a warning for each test defined using
> > > >> > >   REGISTER_TEST_COMMAND
> > > >> > > * In checkpatches, add a check to prevent the addition of new tests
> > > >> > >   using the REGISTER_TEST_COMMAND macro
> > > >> > >
> > > >> > > Bruce Richardson (2):
> > > >> > >   app/test: emit warning for tests not in a test suite
> > > >> > >   devtools: check for tests added without a test suite
> > > >> > >
> > > >> > >  app/test/suites/meson.build   | 13 ++++++++++++-
> > > >> > >  buildtools/get-test-suites.py | 12 +++++++++---
> > > >> > >  devtools/checkpatches.sh      |  8 ++++++++
> > > >> > >  3 files changed, 29 insertions(+), 4 deletions(-)
> > > >> >
> > > >> > The "non_suite_tests" testsuite returned by
> > > >> > buildtools/get-test-suites.py is a bit strange, as it is not a
> > > >> > testsuite from meson pov.
> > > >>
> > > >> Yeah, it is a bit strange, and I'm open to new ideas on other solutions. I
> > > >> did it that way to avoid having yet another script to scan the files - I
> > > >> figured it was faster (in terms of runtime, not dev time) to do the
> > > >
> > > > I had figured it was "faster dev time" that won :-).
> > > > I am fine with it, I don't expect more complications in this area in the future.
> > > >
> > > >
> > > >> scanning when the files are already being opened and processed by this one.
> > > >>
> > > >> Of course, if we can get the un-suitened [:-)] test cases down to zero, we
> > > >> can theoretically drop this check in future, and  just use the checkpatch
> > > >> one.
> > > >
> > > > Well, that's still a question that nobody seems to comment on.
> > > >
> > > > What should we do with tests that don't enter one of those testsuites,
> > > > and are not invoked by the CI?
> > > >
> > > > Though we may be removing some level of coverage, I am for
> > > > cleaning/unused dead code.
> > >
> > > I guess it does require actually looking at these tests and classifying
> > > them to either put them into the proper suites.  As of now, we aren't
> > > really removing coverage if they aren't being run - but are any
> > > maintainers or developers actually running them?
> >
> > Could we go a step further than Bruce runtime warning (which is at the
> > meson level and does not impact running the test)?
> > Perhaps have those orphaned tests fail unless their test names are
> > provided in a env variable like
> > DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard
> > ;-))?
> >
> > With a systematic failure, there is less chance that
> > developers/maintainers miss the situation.
> > If those developers/maintainers simply waive the warning with the env
> > variable and don't send a patch, well.. too bad.
> >
> > After a release or two, if we don't hear from anyone, we can start
> > removing the unused one.
> >
> I think that seems a littel severe at this point.

I'll put this idea in the fridge for now.
Let's proceed with your series first (and some doc followups).


-- 
David Marchand

Re: [PATCH 0/2] add checks for tests not in a suite

[David Marchand]

On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> To help ensure that we don't have "orphaned" tests not in any test
> suites we can add the following checks:
>
> * In developer-mode builds, emit a warning for each test defined using
>   REGISTER_TEST_COMMAND
> * In checkpatches, add a check to prevent the addition of new tests
>   using the REGISTER_TEST_COMMAND macro
>
> Bruce Richardson (2):
>   app/test: emit warning for tests not in a test suite
>   devtools: check for tests added without a test suite
>
>  app/test/suites/meson.build   | 13 ++++++++++++-
>  buildtools/get-test-suites.py | 12 +++++++++---
>  devtools/checkpatches.sh      |  8 ++++++++
>  3 files changed, 29 insertions(+), 4 deletions(-)

Series applied.
Thanks Bruce.


-- 
David Marchand