Bruce Richardson <bruce.richard...@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.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.
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
