Re: [OE-core][PATCHv2 1/2] classes/ptest-python-pytest: add a new class to consolidate pytest ptest functionality

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Derek,

On 12/18/24 12:12 AM, Derek Straka via lists.openembedded.org wrote:
> A large number of python packages leverage the pytest unit test
> framework for their ptest functionality.  Currently, many of the tests
> have duplicate code for:
>      1. Installing pytest files
>      2. Declaring ptest dependencies
>      3. Script for executing tests (run-ptes)
> 
> To simplify adding common pytest based ptests, added a new class
> enabling base functionality.  Users can also override the location of
> the pytest files in addition to using their own version of run-ptest
> 
> Signed-off-by: Derek Straka <derek@asterius.io>
> ---
>   .../ptest-python-pytest.bbclass               | 42 +++++++++++++++++++
>   meta/files/ptest-python-pytest/run-ptest      |  3 ++
>   2 files changed, 45 insertions(+)
>   create mode 100644 meta/classes-recipe/ptest-python-pytest.bbclass
>   create mode 100755 meta/files/ptest-python-pytest/run-ptest
> 
> diff --git a/meta/classes-recipe/ptest-python-pytest.bbclass b/meta/classes-recipe/ptest-python-pytest.bbclass
> new file mode 100644
> index 0000000000..89ff10c335
> --- /dev/null
> +++ b/meta/classes-recipe/ptest-python-pytest.bbclass
> @@ -0,0 +1,42 @@
> +#
> +# Copyright OpenEmbedded Contributors
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +
> +inherit ptest
> +
> +FILESEXTRAPATHS:prepend := "${COREBASE}/meta/files:"

I think it'd make sense to use :append here as we don't want to override 
anything, just provide a fallback in case the 
ptest-python-pytest/run-ptest isn't find anywhere else. This would also 
allow to easily override this file by just adding the file next to the 
recipe, instead of having to add a FILESEXTRAPATHS:prepend := 
"${THISDIR}/${PN}:" in the recipe after the inherit for example.

> +
> +SRC_URI += "file://ptest-python-pytest/run-ptest"
> +

We use append/prepend everywhere but here, should we?

> +# Overridable configuration for the directory within the source tree
> +# containing the pytest files
> +PTEST_PYTEST_DIR ?= "/tests"
> +
> +do_install_ptest_python_pytest() {
> +	if [ ! -f ${D}${PTEST_PATH}/run-ptest ]; then
> +		install -m 0755 ${UNPACKDIR}/ptest-python-pytest/run-ptest ${D}${PTEST_PATH}
> +	fi

This means if we ever built the recipe and don't clean its WORKDIR, but 
update the file, it will never be updated. I don't think that's what we 
want. Can you explain what you're trying to prevent here by having a 
check for NOT installing the file?

> +    if [ -d "${S}/${PTEST_PYTEST_DIR}" ]; then

Shouldn't we use UNPACKDIR for file:// SRC_URI in master?

Cheers,
Quentin