From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2567442653; Wed, 27 Sep 2023 16:00:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EEBF1402AA; Wed, 27 Sep 2023 16:00:19 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id C652B4028C for ; Wed, 27 Sep 2023 16:00:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695823217; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fcDzMNJtwXa+Say/28K6VWrHACWvwl5nPdSAJrd8+FM=; b=F60rGcFd1KQsxpb3/Dck+cgIb98y8Ju7k8O8w2W8QotvUl9IMUJEl3mqHmW9JQdJlBIaoa 1nJ5HNyHUuJA3maMXou8Wfc8HB9FXcA+6xKyr9qiIpTmjQcuy1ACHxwz5X+f/MBQg7HRIH Dro4n5Ws6CICgKMHATbEnchC8Wq0QpQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-593-G0DjR4hlNh6mpeksTgetIQ-1; Wed, 27 Sep 2023 10:00:13 -0400 X-MC-Unique: G0DjR4hlNh6mpeksTgetIQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A4AB4800045; Wed, 27 Sep 2023 14:00:12 +0000 (UTC) Received: from RHTPC1VM0NT (unknown [10.22.10.131]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0A46D40C6E76; Wed, 27 Sep 2023 14:00:11 +0000 (UTC) From: Aaron Conole To: Bruce Richardson Cc: David Marchand , , Thomas Monjalon , Ferruh Yigit , "Jerin Jacob Kollanukkaran" , Akhil Goyal , Maxime Coquelin Subject: Re: [PATCH 0/2] add checks for tests not in a suite References: <20230915115206.132198-1-bruce.richardson@intel.com> Date: Wed, 27 Sep 2023 10:00:10 -0400 In-Reply-To: (Bruce Richardson's message of "Wed, 27 Sep 2023 10:56:03 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Bruce Richardson writes: > On Wed, Sep 27, 2023 at 11:31:07AM +0200, David Marchand wrote: >> On Wed, Sep 27, 2023 at 10:27=E2=80=AFAM Bruce Richardson >> wrote: >> > On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote: >> > > On Tue, Sep 26, 2023 at 5:01=E2=80=AFPM Aaron Conole wrote: >> > > > David Marchand writes: >> > > > > On Tue, Sep 19, 2023 at 10:36=E2=80=AFAM Bruce Richardson >> > > > > wrote: >> > > > >> > > To help ensure that we don't have "orphaned" tests not in a= ny test >> > > > >> > > suites we can add the following checks: >> > > > >> > > >> > > > >> > > * In developer-mode builds, emit a warning for each test de= fined using >> > > > >> > > REGISTER_TEST_COMMAND >> > > > >> > > * In checkpatches, add a check to prevent the addition of n= ew 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 s= olutions. 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 are= a 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 tests= uites, >> > > > > 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 classi= fying >> > > > them to either put them into the proper suites. As of now, we are= n'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 t= he >> > > 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 en= v >> > > 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 explai= ning >> > what the various suite types are, so that developers can choose the ri= ght >> > one for their test(s). We may even need a couple more if some tests d= o not >> > fit into the existing categories. Once that is done, we should then st= art >> > looking for tests that are obsolete and can be removed. >> > >> > If we do already have documentation on the various suites and how to u= se >> > them, apologies for my ignorance, and perhaps someone could post a lin= k >> > here. >>=20 >> We have some guidelines but I agree the testsuites are not described. >> https://doc.dpdk.org/guides/contributing/unit_test.html >>=20 > I also see I missed updating the reference to REGISTER_TEST_COMMAND in th= e > 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