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.richard...@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 <acon...@redhat.com> wrote: > > > > David Marchand <david.march...@redhat.com> writes: > > > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson > > > > > <bruce.richard...@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
