Re: [PATCH net-next v4 1/4] selftests: net: py: add metric output

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In tools/testing/selftests/net/lib/py/ksft.py, the ksft_metric()
  docstring ("They do not affect the result.") and…
- [Low] ksft_metric() in tools/testing/selftests/net/lib/py/ksft.py does
  not check that `goal` is one of "higher", "lower" or "zero", or that…
- [Low] tools/testing/selftests/drivers/net/README.rst now documents
  ksft_metric() under "Guidance for test authors", next to ksft_pr(),…

--

selftests: net: py: add metric output

This commit adds ksft_metric() to the Python net selftest library. The
helper prints a numeric measurement of the current test case as a compact
JSON line prefixed with "ktap-metric-json:", tagged with a goal of higher,
lower or zero. It is exported from net/lib/py and documented in
drivers/net/README.rst.

> diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> index 3fe49bce4f3a5..f651163c16c8c 100644
> --- a/tools/testing/selftests/drivers/net/README.rst
> +++ b/tools/testing/selftests/drivers/net/README.rst
> @@ -259,6 +259,22 @@ ksft_pr()
>  
>  Use ``ksft_pr()`` instead of ``print()`` to avoid breaking TAP format.
>  
> +ksft_metric()
> +~~~~~~~~~~~~~
> +
> +Use ``ksft_metric()`` to report a numeric measurement of the current test
> +case. Metrics are printed right away as compact JSON. For example::

[Severity: Low]
Can tests in drivers/net/ and drivers/net/hw/ import this helper the way
the README suggests?

This section sits next to ksft_pr(), defer(), ksft_disruptive and
ksft_variants. All of those are re-exported by
drivers/net/lib/py/__init__.py and drivers/net/hw/lib/py/__init__.py.
Both files use an explicit import list and an explicit __all__:

    from net.lib.py import ksft_disruptive, ksft_exit, ksft_pr, ksft_run, \
        ksft_setup, ksft_variants, KsftNamedVariant

This patch only adds ksft_metric to net/lib/py/__init__.py. So a test in
drivers/net/ or drivers/net/hw/ that does "from lib.py import ksft_metric"
would fail with ImportError when the module loads.

Later patches in the series add drivers/net/perf/lib/py/__init__.py,
which does export ksft_metric. The drivers/net and drivers/net/hw libs
still don't export it at the end of the series.

Should those two re-export lists be updated as well?

[ ... ]

> diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
> index 81287c2daff09..572ee17b4f4f2 100644
> --- a/tools/testing/selftests/net/lib/py/ksft.py
> +++ b/tools/testing/selftests/net/lib/py/ksft.py

[ ... ]

> @@ -93,6 +94,25 @@ def ksft_pr(*objs, **kwargs):
>      print(pfx, prefixed, **kwargs)
>  
>  
> +def ksft_metric(name, value, *, goal):
> +    """
> +    Report a numeric measurement of the current test case.
> +
> +    Metrics are printed right away. They do not affect the result.
> +
> +    Args:
> +      name - name of the metric, e.g. "throughput"
> +      value - the measured number
> +      goal - "higher" or "lower" for a performance metric, depending on
> +             which way is better, "zero" for an error metric, which must
> +             be zero
> +    """
> +    metric = {"name": name, "value": value, "goal": goal}

[Severity: Low]
This isn't a bug, but should ksft_metric() check that goal is one of
"higher", "lower" or "zero", and that value is a number?

Right now, a misspelled goal, a string or None value, or a bool (which
is encoded as true) still produces a well-formed ktap-metric-json line.
A consumer may not be able to classify that line.

The callers added later in the series, drivers/net/perf/iperf.py and
drivers/net/perf/devmem.py, both pass goal="higher" with a float
throughput. So this would only catch mistakes in future callers.

> +    payload = json.dumps(metric, allow_nan=False, separators=(",", ":"),
> +                         sort_keys=True)

[Severity: Low]
Is "They do not affect the result" true for every input?

With allow_nan=False, json.dumps() raises ValueError for NaN or
+/-Infinity, and TypeError for values it cannot encode. ksft_metric()
does not catch these, so they reach the generic handler in ksft_run():

        except BaseException as e:
            ...
            KSFT_RESULT = False
            cnt_key = 'fail'

The test case is then reported as failed. The README makes the same
claim: "Metrics do not affect the result of the test."

The callers in the series always pass finite throughput values. Also,
allow_nan=False looks like a deliberate choice to keep the output strict
JSON.

Could the docstring and the README say that non-finite or non-numeric
values raise an exception and fail the test case?

> +    ksft_pr(f"ktap-metric-json: {payload}")

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111000246.962871-1-sdf%40fomichev.me