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 E46F045CFB; Wed, 13 Nov 2024 22:44:16 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D2902410DD; Wed, 13 Nov 2024 22:44:16 +0100 (CET) Received: from mail-yw1-f169.google.com (mail-yw1-f169.google.com [209.85.128.169]) by mails.dpdk.org (Postfix) with ESMTP id 4FF26410DC for ; Wed, 13 Nov 2024 22:44:16 +0100 (CET) Received: by mail-yw1-f169.google.com with SMTP id 00721157ae682-6ea5003deccso80903217b3.0 for ; Wed, 13 Nov 2024 13:44:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1731534255; x=1732139055; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=6uk95LOgHMMxPtW5AVXXeH2AzVxeEk4HTN1QxmsUJrs=; b=KwzmVdgx6CGSLEqEmkt9Nani/SfhGZFyz5HOcRuqttpgQaqbelLRakTDezvD6deyxa +WpUTq3wv9SYvzDlxsa5jLmd3bTJJtc9zUtF+u0aQDcXnDVzIfrlyXUzn6grZIJVMasG DqbjGBuw+hmOFlST+pZmq0rIrv1DL+yx/Uw5I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731534255; x=1732139055; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=6uk95LOgHMMxPtW5AVXXeH2AzVxeEk4HTN1QxmsUJrs=; b=LKyJiI2u+cJV9Gmj5C454Jc1cPMbz6Ih2ATmnU7fUSQOHjBubkzh24IAIeWeyiH+QT KeoC1tgLvOhXg0khgSGTPVkTKMmLVvzenKaj+M1w+SOKcQd4QA1wsM/hopp4TAWsg+9Z m2eyA0lwJarpai3x1HdV3Pp9N6cysQD45m1ygPD4EYnAULfpZgwgzawyk69sfkzutYek /SpD1swjkmTVhWXY98EsCZr+RFddtX50dTtTHbXAZwgaVyuIxBOuBLpsM/HeluL67ObK lKek/NpFrBUpHZ90VyOn18l0Xa6Gos0j+qSGSxcJ0qgRFxPekQk4KDo7Mdsnw+e+5pVK tL/A== X-Gm-Message-State: AOJu0Yx/6fT8SH9LN2bjyfyH5SBd0Fh1gqsWNvI0JHkyZCAFJKsv7ic9 4h7SVfrIQMYimTJqotbP2+fH6tWvHrCqVUfUIPxvDsG4OKvYGeTRVo4HB5S9e3n2Sq07f6tUaT7 9XW0yXIyEybXniB9gQlfH+Q3ObF6QDLDfQlxqhw== X-Google-Smtp-Source: AGHT+IGPEmxsFwDgsF6X73U74WAXrBc8lB8IR3vGKb2n6xZjPrFG4uMitOpkQRNcNepn117VQDQf2OE/u49LVA9YRTg= X-Received: by 2002:a05:690c:6f8f:b0:6ea:7c46:8c23 with SMTP id 00721157ae682-6eaddfb0191mr249242047b3.35.1731534255607; Wed, 13 Nov 2024 13:44:15 -0800 (PST) MIME-Version: 1.0 References: <20240906161355.701688-1-luca.vizzarro@arm.com> <20241108134532.130681-1-luca.vizzarro@arm.com> <20241108134532.130681-4-luca.vizzarro@arm.com> In-Reply-To: <20241108134532.130681-4-luca.vizzarro@arm.com> From: Dean Marx Date: Wed, 13 Nov 2024 16:44:28 -0500 Message-ID: Subject: Re: [PATCH v3 3/4] dts: add per-test-suite configuration To: Luca Vizzarro Cc: dev@dpdk.org, Paul Szczepanek , Patrick Robb Content-Type: multipart/alternative; boundary="00000000000038ed0b0626d23cf3" 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 --00000000000038ed0b0626d23cf3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > > On Fri, Nov 8, 2024 at 8:38=E2=80=AFAM Luca Vizzarro > wrote: > >> Allow test suites to be configured individually. Moreover enable them to >> implement their own custom configuration. >> >> This solution adds some new complexity to DTS, which is generated source >> code. In order to ensure strong typing, the test suites and their custom >> configurations need to be linked in the main configuration class. >> Unfortunately, this is not feasible during runtime as it will incur in >> circular dependencies. Generating the links appear to be the most >> straightforward approach. >> >> This commit also brings a new major change to the configuration schema. >> Test suites are no longer defined as a list of strings, like: >> >> test_suites: >> - hello_world >> - pmd_buffer_scatter >> >> but as mapping of mappings or strings: >> >> test_suites: >> hello_world: {} # any custom fields or test cases can be set here >> pmd_buffer_scatter: all # "all" defines all the test cases, or >> # they can individually be set separated >> # by a space >> >> Not defining the `test_cases` field in the configuration is equivalent >> to `all`, therefore the definitions for either test suite above are >> also equivalent. >> >> Creating the __init__.py file under the tests folder, allows it to be >> picked up as a package. This is a mypy requirement to import the tests >> from within the framework. >> >> Bugzilla ID: 1375 >> >> Signed-off-by: Luca Vizzarro >> Reviewed-by: Paul Szczepanek >> > > I like the idea of mapping the suite to specific test cases, and for the > most part the custom configuration option as well. The only thing that I > feel should be different is the way the code generation is documented, I > think it might be worth providing an example within conf.yaml through a > comment near the suites section, rather than just in the dts.rst file. It > might be a little more clear where to create the custom config class as > well. > > > >> +class HelloWorldConfig(TestSuiteConfig): >> + """Example custom configuration for the `TestHelloWorld` test >> suite.""" >> + >> + #: Timeout for the DPDK apps. >> + timeout: int =3D 50 >> -- >> 2.43.0 >> >> > Additionally, I was a bit confused by the custom config examples, do thes= e > fields (timeout, my_custom_field) actually affect the suite in any way as > of this patch? Or is this just so that we can potentially add configurati= on > options through this method in the future? > > Reviewed-by: Dean Marx > --00000000000038ed0b0626d23cf3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Fri, Nov 8, 2024 at 8:38=E2=80= =AFAM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
Allow test suites t= o be configured individually. Moreover enable them to
implement their ow= n custom configuration.

This solution adds some new complexity to DT= S, which is generated source
code. In order to ensure strong typing, the= test suites and their custom
configurations need to be linked in the ma= in configuration class.
Unfortunately, this is not feasible during runti= me as it will incur in
circular dependencies. Generating the links appea= r to be the most
straightforward approach.

This commit also bring= s a new major change to the configuration schema.
Test suites are no lon= ger defined as a list of strings, like:

=C2=A0 =C2=A0 test_suites:=C2=A0 =C2=A0 - hello_world
=C2=A0 =C2=A0 - pmd_buffer_scatter

= but as mapping of mappings or strings:

=C2=A0 =C2=A0 test_suites:=C2=A0 =C2=A0 =C2=A0 hello_world: {} # any custom fields or test cases can= be set here
=C2=A0 =C2=A0 =C2=A0 pmd_buffer_scatter: all # "all&qu= ot; defines all the test cases, or
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # the= y can individually be set separated
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # by = a space

Not defining the `test_cases` field in the configuration is = equivalent
to `all`, therefore the definitions for either test suite abo= ve are
also equivalent.

Creating the __init__.py file under the t= ests folder, allows it to be
picked up as a package. This is a mypy requ= irement to import the tests
from within the framework.

Bugzilla I= D: 1375

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-b= y: Paul Szczepanek <paul.szczepanek@arm.com>

I like the idea of mapping the suite to specific test cases, and for the = most part the custom configuration option as well. The only thing that I fe= el should be different is the way the code generation is documented, I thin= k it might be worth providing an example within conf.yaml through a comment= near the suites section, rather than just in the dts.rst file. It might be= a little more clear where to create the custom config class as well.=C2=A0=

=C2=A0<snip>
+class HelloWorldConfig(TestSuiteConfig):
+=C2= =A0 =C2=A0 """Example custom configuration for the `TestHell= oWorld` test suite."""
+
+=C2=A0 =C2=A0 #: Timeout for= the DPDK apps.
+=C2=A0 =C2=A0 timeout: int =3D 50
--
2.43.0

Additionally, I was a bit confused by th= e custom config examples, do these fields (timeout, my_custom_field) actual= ly affect the suite in any way as of this patch? Or is this just so that we= can potentially add configuration options through this method in the futur= e?

Reviewed-by: Dean Marx <dmarx@iol.unh.edu>=C2=A0
--00000000000038ed0b0626d23cf3--