From: David Marchand <david.marchand@redhat.com>
To: Aaron Conole <aconole@redhat.com>,
Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>,
Ferruh Yigit <ferruh.yigit@amd.com>,
Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
Akhil Goyal <gakhil@marvell.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>
Subject: Re: [PATCH 0/2] add checks for tests not in a suite
Date: Wed, 27 Sep 2023 08:30:05 +0200 [thread overview]
Message-ID: <CAJFAV8w9ZFb53nbf7VCXjuexmdNdGZSFMpXJQR6cSeojJaAgvg@mail.gmail.com> (raw)
In-Reply-To: <f7tr0mlrnai.fsf@redhat.com>
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
next prev parent reply other threads:[~2023-09-27 6:30 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-15 11:52 Bruce Richardson
2023-09-15 11:52 ` [PATCH 1/2] app/test: emit warning for tests not in a test suite Bruce Richardson
2023-09-15 11:52 ` [PATCH 2/2] devtools: check for tests added without " Bruce Richardson
2023-09-15 11:55 ` [PATCH 0/2] add checks for tests not in a suite Bruce Richardson
2023-09-19 8:29 ` David Marchand
2023-09-19 8:36 ` Bruce Richardson
2023-09-19 9:07 ` David Marchand
2023-09-26 15:01 ` Aaron Conole
2023-09-27 6:30 ` David Marchand [this message]
2023-09-27 8:26 ` Bruce Richardson
2023-09-27 9:31 ` David Marchand
2023-09-27 9:56 ` Bruce Richardson
2023-09-27 14:00 ` Aaron Conole
2023-10-02 9:35 ` David Marchand
2023-10-02 9:35 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJFAV8w9ZFb53nbf7VCXjuexmdNdGZSFMpXJQR6cSeojJaAgvg@mail.gmail.com \
--to=david.marchand@redhat.com \
--cc=aconole@redhat.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=gakhil@marvell.com \
--cc=jerinj@marvell.com \
--cc=maxime.coquelin@redhat.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).