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.
I'll put this idea in the fridge for now. Let's proceed with your series first (and some doc followups). -- David Marchand
