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 C41C34233F; Mon, 9 Oct 2023 19:17:01 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9F97F402B1; Mon, 9 Oct 2023 19:17:01 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 438EA4026B; Mon, 9 Oct 2023 19:17:00 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id E21B726687; Mon, 9 Oct 2023 19:16:59 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id B842E2642D; Mon, 9 Oct 2023 19:16:59 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.5 required=5.0 tests=ALL_TRUSTED,AWL autolearn=disabled version=3.4.6 X-Spam-Score: -1.5 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id E739026520; Mon, 9 Oct 2023 19:16:58 +0200 (CEST) Message-ID: <4c8f3685-e719-4a56-a57e-5ba8c3ae18c1@lysator.liu.se> Date: Mon, 9 Oct 2023 19:16:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/3] test: add dispatcher test suite Content-Language: en-US To: David Marchand Cc: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org, Jerin Jacob , techboard@dpdk.org, harry.van.haaren@intel.com, Peter Nilsson , Heng Wang , Naga Harish K S V , Pavan Nikhilesh , Gujjar Abhinandan S , Erik Gabriel Carrillo , Shijith Thotton , Hemant Agrawal , Sachin Saxena , Liang Ma , Peter Mccarthy , Zhirun Yan References: <20230922073825.351453-2-mattias.ronnblom@ericsson.com> <20230928073056.359356-1-mattias.ronnblom@ericsson.com> <20230928073056.359356-3-mattias.ronnblom@ericsson.com> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 On 2023-10-06 10:52, David Marchand wrote: > On Thu, Oct 5, 2023 at 1:26 PM Mattias Rönnblom wrote: > > [snip] > >>>> +#define RETURN_ON_ERROR(rc) \ >>>> + do { \ >>>> + if (rc != TEST_SUCCESS) \ >>>> + return rc; \ >>>> + } while (0) >>> >>> TEST_ASSERT? >>> This gives context about which part of a test failed. >>> >> >> This macro is used in a situation where the failure has occured and has >> been reported already. >> >> Maybe it would be better to replace the macro instationation with just >> the if+return statements. >> >> RETURN_ON_ERROR(rc); >> >> -> >> >> if (rc != TEST_SUCCESS) >> return rc; > > Yes, this macro does not add much, you can remove it. > OK, will do. > [snip] > > >>>> + for (i = 0; i < NUM_SERVICE_CORES; i++) >>>> + if (app->service_lcores[i] == lcore_id) >>>> + return i; >>> >>> This construct is hard to read and prone to error if the code is updated later. >>> >>> for () { >>> if () >>> return i; >>> } >>> >>> >> >> I wouldn't consider that an improvement (rather the opposite). > > Well, I disagree, but it is not enforced in the coding style so I won't insist. > > [snip] > > >>>> +static struct unit_test_suite test_suite = { >>>> + .suite_name = "Event dispatcher test suite", >>>> + .unit_test_cases = { >>>> + TEST_CASE_ST(test_setup, test_teardown, test_basic), >>>> + TEST_CASE_ST(test_setup, test_teardown, test_drop), >>>> + TEST_CASE_ST(test_setup, test_teardown, >>>> + test_many_handler_registrations), >>>> + TEST_CASE_ST(test_setup, test_teardown, >>>> + test_many_finalize_registrations), >>>> + TEST_CASES_END() >>>> + } >>>> +}; >>>> + >>>> +static int >>>> +test_dispatcher(void) >>>> +{ >>>> + return unit_test_suite_runner(&test_suite); >>>> +} >>>> + >>>> +REGISTER_TEST_COMMAND(dispatcher_autotest, test_dispatcher); >>> >>> We have new macros (see REGISTER_FAST_TEST for example) so a test is >>> associated to an existing testsuite. >>> I think this test should be part of the fast-test testsuite, wdyt? >>> >>> >> >> It needs setup and teardown methods, so I assume a generic test suite >> woulnd't do. >> >> The dispatcher tests do have fairly short run times, so in that sense >> they should qualify. > > > So please use REGISTER_FAST_TEST(). > Thanks. > > OK.