Message-ID: <CA+GJov5Ug5_xi7WDd1RQScR4cXya9LqDZ-5zKREpaRp2PoWB1A@mail.gmail.com>
Date: Mon, 5 Feb 2024 23:12:16 -0500
From: Rae Moar <rmoar@...gle.com>
To: David Gow <davidgow@...gle.com>
Cc: frowand.list@...il.com, keescook@...omium.org, Tim.Bird@...y.com, 
	shuah@...nel.org, brendanhiggins@...gle.com, dlatypov@...gle.com, 
	tytso@...gle.com, gustavo.padovan@...labora.com, 
	ricardo.canuelo@...labora.com, guillaume.tucker@...labora.com, corbet@....net, 
	kernelci@...ts.linux.dev, linux-kselftest@...r.kernel.org, 
	kunit-dev@...glegroups.com, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [KTAP V2 PATCH v2] ktap_v2: add test metadata

On Sat, Feb 3, 2024 at 1:50 AM David Gow <davidgow@...gle.com> wrote:
>
> On Sat, 27 Jan 2024 at 06:15, Rae Moar <rmoar@...gle.com> wrote:
> >
> > Add specification for test metadata to the KTAP v2 spec.
> >
> > KTAP v1 only specifies the output format of very basic test information:
> > test result and test name. Any additional test information either gets
> > added to general diagnostic data or is not included in the output at all.
> >
> > The purpose of KTAP metadata is to create a framework to include and
> > easily identify additional important test information in KTAP.
> >
> > KTAP metadata could include any test information that is pertinent for
> > user interaction before or after the running of the test. For example,
> > the test file path or the test speed.
> >
> > Since this includes a large variety of information, this specification
> > will recognize notable types of KTAP metadata to ensure consistent format
> > across test frameworks. See the full list of types in the specification.
> >
> > Example of KTAP Metadata:
> >
> >  KTAP version 2
> >  # ktap_test: main
> >  # ktap_arch: uml
> >  1..1
> >      KTAP version 2
> >      # ktap_test: suite_1
> >      # ktap_subsystem: example
> >      # ktap_test_file: lib/test.c
> >      1..2
> >      ok 1 test_1
> >      # ktap_test: test_2
> >      # ktap_speed: very_slow
> >      # custom_is_flaky: true
> >      ok 2 test_2
> >  ok 1 test_suite
>
> This 'test_suite' name doesn't match the 'suite_1' name above.
>

Hello!

Thanks for your review of this documentation. And sorry about that
typo. I will change that in the next version.

> It also could be clearer that it's supposed to match 'suite_1', not
> 'main', due to the indentation difference. Maybe we should add an
> explicit note pointing that out?

This is a good point. I will add a note in the specification example.

>
> >
> > The changes to the KTAP specification outline the format, location, and
> > different types of metadata.
> >
> > Here is a link to a version of the KUnit parser that is able to parse test
> > metadata lines for KTAP version 2. Note this includes test metadata
> > lines for the main level of KTAP.
> >
> > Link: https://kunit-review.googlesource.com/c/linux/+/5889
>
> I tested this, and it works well. I think there's a couple of changes
> we'd want for a more useful set of KUnit parser changes (namely the
> option to support non ktap_ prefixes, as well as an actual way of
> using this data), but I'll leave those for a future review of that
> patch -- it's not relevant to this spec.

Thanks for testing this! My thought was to only support ktap_ prefixes
in the parser for now and we could add on others as needed. I will
make a separate patch series for this once KTAP Metadata goes through.

>
> >
> > Signed-off-by: Rae Moar <rmoar@...gle.com>
> > ---
>
> I like this: it covers all of the requirements we have in KUnit, as
> well as a few things we'd like to add.
>
> Is there anything obviously missing for this to work with other
> usecases? Are there any other examples of metadata people want to
> capture?
>

Yes, I am also curious about what other use cases people are
interested in as well?

> For me, this is
> Reviewed-by: David Gow <davidgow@...gle.com>
>
> >  Documentation/dev-tools/ktap.rst | 163 ++++++++++++++++++++++++++++++-
> >  1 file changed, 159 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/ktap.rst b/Documentation/dev-tools/ktap.rst
> > index ff77f4aaa6ef..4480eaf5bbc3 100644
> > --- a/Documentation/dev-tools/ktap.rst
> > +++ b/Documentation/dev-tools/ktap.rst
> > @@ -17,19 +17,20 @@ KTAP test results describe a series of tests (which may be nested: i.e., test
> >  can have subtests), each of which can contain both diagnostic data -- e.g., log
> >  lines -- and a final result. The test structure and results are
> >  machine-readable, whereas the diagnostic data is unstructured and is there to
> > -aid human debugging.
> > +aid human debugging. One exception to this is test metadata lines - a type
> > +of diagnostic lines. Test metadata is used to identify important supplemental
> > +test information and can be machine-readable.
> >
> >  KTAP output is built from four different types of lines:
> >  - Version lines
> >  - Plan lines
> >  - Test case result lines
> > -- Diagnostic lines
> > +- Diagnostic lines (including test metadata)
> >
> >  In general, valid KTAP output should also form valid TAP output, but some
> >  information, in particular nested test results, may be lost. Also note that
> >  there is a stagnant draft specification for TAP14, KTAP diverges from this in
> > -a couple of places (notably the "Subtest" header), which are described where
> > -relevant later in this document.
> > +a couple of places, which are described where relevant later in this document.
> >
> >  Version lines
> >  -------------
> > @@ -166,6 +167,154 @@ even if they do not start with a "#": this is to capture any other useful
> >  kernel output which may help debug the test. It is nevertheless recommended
> >  that tests always prefix any diagnostic output they have with a "#" character.
> >
> > +KTAP metadata lines
> > +-------------------
> > +
> > +KTAP metadata lines are a subset of diagnostic lines that are used to include
> > +and easily identify important supplemental test information in KTAP.
> > +
> > +.. code-block:: none
> > +
> > +       # <prefix>_<metadata type>: <metadata value>
> > +
> > +The <prefix> indicates where to find the specification for the type of
> > +metadata. The metadata types listed below use the prefix "ktap" (See Types of
> > +KTAP Metadata).
> > +
> > +Types that are instead specified by an individual test framework use the
> > +framework name as the prefix. For example, a metadata type documented by the
> > +kselftest specification would use the prefix "kselftest". Any metadata type
> > +that is not listed in a specification must use the prefix "custom". Note the
> > +prefix must not include spaces or the characters ":" or "_".
>
> We should probably be more explicit about what counts as a
> 'specification' here, and hence whether a new prefix or 'custom'
> should be used.
>
> I'm tempted to be inspired by the OpenGL extension mechanism and say
> that new prefixes must be 'registered' before they can be used, where
> 'registration' consists of submitting a patch to this document linking
> to the specification.

Oh, I really like this idea. I could add a current list of supported
prefixes and even a link to the documentation of those metadata types.

>
> > +The format of <metadata type> and <value> varies based on the type. See the
> > +individual specification. For "custom" types the <metadata type> can be any
> > +string excluding ":", spaces, or newline characters and the <value> can be any
> > +string.
> > +
> > +**Location:**
> > +
> > +The first KTAP metadata entry for a test must be "# ktap_test: <test name>",
> > +which acts as a header to associate metadata with the correct test.
> > +
> > +For test cases, the location of the metadata is between the prior test result
> > +line and the current test result line. For test suites, the location of the
> > +metadata is between the suite's version line and test plan line. See the
> > +example below.
> > +
> > +KTAP metadata for a test does not need to be contiguous. For example, a kernel
> > +warning or other diagnostic output could interrupt metadata lines. However, it
> > +is recommended to keep a test's metadata lines together when possible, as this
> > +improves readability.
>
> Should we give an example of this? e.g. ktap_duration will need to be
> output after the test has completed, but ktap_test must be output
> before any log lines. (And most of the others probably prefer to be at
> the beginning.)
>
> I think this is the most complicated point from a parsing point of
> view, so we should probably draw more attention to it.

I will try to add an example here to flush out this concept including
ktap_test and ktap_duration.

>
> > +
> > +**Here is an example of using KTAP metadata:**
> > +
> > +::
> > +
> > +        KTAP version 2
> > +        # ktap_test: main
> > +        # ktap_arch: uml
> > +        1..1
> > +          KTAP version 2
> > +          # ktap_test: suite_1
> > +          # ktap_subsystem: example
> > +          # ktap_test_file: lib/test.c
> > +          1..2
> > +          ok 1 test_1
> > +          # ktap_test: test_2
> > +          # ktap_speed: very_slow
> > +          # custom_is_flaky: true
> > +          ok 2 test_2
> > +        # suite_1 passed
> > +        ok 1 suite_1
>
> Would it be clearer to have some examples which have other,
> non-metadata diagnostic lines here, so we can see how they interact?
>

I definitely see this point. I can either add non-metadata diagnostic
lines to this example or I could also add a separate example. I am
leaning towards the former but let me know if you would have a strong
preference for a separate example instead.

> > +
> > +In this example, the tests are running on UML. The test suite "suite_1" is part
> > +of the subsystem "example" and belongs to the file "lib/example_test.c". It has
> > +two subtests, "test_1" and "test_2". The subtest "test_2" has a speed of
> > +"very_slow" and has been marked with a custom KTAP metadata type called
> > +"custom_is_flaky" with the value of "true".
> > +
> > +**Types of KTAP Metadata:**
> > +
> > +This is the current list of KTAP metadata types recognized in this
> > +specification. Note that all of these metadata types are optional (except for
> > +ktap_test as the KTAP metadata header).
> > +
> > +- ``ktap_test``: Name of test (used as header of KTAP metadata). This should
> > +  match the test name printed in the test result line: "ok 1 [test_name]".
> > +
> > +- ``ktap_module``: Name of the module containing the test
> > +
> > +- ``ktap_subsystem``: Name of the subsystem being tested
> > +
> > +- ``ktap_start_time``: Time tests started in ISO8601 format
> > +
> > +  - Example: "# ktap_start_time: 2024-01-09T13:09:01.990000+00:00"
> > +
> > +- ``ktap_duration``: Time taken (in seconds) to execute the test
> > +
> > +  - Example: "ktap_duration: 10.154s"
> > +
> > +- ``ktap_speed``: Category of how fast test runs: "normal", "slow", or
> > +  "very_slow"
> > +
> > +- ``ktap_test_file``: Path to source file containing the test. This metadata
> > +  line can be repeated if the test is spread across multiple files.
> > +
> > +  - Example: "# ktap_test_file: lib/test.c"
> > +
> > +- ``ktap_generated_file``: Description of and path to file generated during
> > +  test execution. This could be a core dump, generated filesystem image, some
> > +  form of visual output (for graphics drivers), etc. This metadata line can be
> > +  repeated to attach multiple files to the test.
> > +
> > +  - Example: "# ktap_generated_file: Core dump: /var/lib/systemd/coredump/hello.core"
> > +
> > +- ``ktap_log_file``: Path to file containing kernel log test output
> > +
> > +  - Example: "# ktap_log_file: /sys/kernel/debugfs/kunit/example/results"
>
> When should we use something generic like 'ktap_generated_file',
> versus something more specific, like 'ktap_log_file'?
>

I think it would be best to use the more specific ktap_log_file when
possible. However, I think it largely depends on how the test author
and test framework want to use these metadata types. I will add a note
here to document that.

Thanks for your thoughts!
-Rae


> > +
> > +- ``ktap_error_file``: Path to file containing context for test failure or
> > +  error. This could include the difference between optimal test output and
> > +  actual test output.
> > +
> > +  - Example: "# ktap_error_file: fs/results/example.out.bad"
> > +
> > +- ``ktap_results_url``: Link to webpage describing this test run and its
> > +  results
> > +
> > +  - Example: "# ktap_results_url: https://kcidb.kernelci.org/hello"
> > +
> > +- ``ktap_arch``: Architecture used during test run
> > +
> > +  - Example: "# ktap_arch: x86_64"
> > +
> > +- ``ktap_compiler``: Compiler used during test run
> > +
> > +  - Example: "# ktap_compiler: gcc (GCC) 10.1.1 20200507 (Red Hat 10.11-1)"
> > +
> > +- ``ktap_respository_url``: Link to git repository of the checked out code.
> > +
> > +  - Example: "# ktap_respository_url: https://github.com/torvalds/linux.git"
> > +
> > +- ``ktap_git_branch``: Name of git branch of checked out code
> > +
> > +  - Example: "# ktap_git_branch: kselftest/kunit"
> > +
> > +- ``ktap_kernel_version``: Version of Linux Kernel being used during test run
> > +
> > +  - Example: "# ktap_kernel_version: 6.7-rc1"
> > +
> > +- ``ktap_commit_hash``: The full git commit hash of the checked out base code.
> > +
> > +  - Example: "# ktap_commit_hash: 064725faf8ec2e6e36d51e22d3b86d2707f0f47f"
> > +
> > +**Other Metadata Types:**
> > +
> > +There can also be KTAP metadata that is not included in the recognized list
> > +above. This metadata must be prefixed with the test framework, ie. "kselftest",
> > +or with the prefix "custom". For example, "# custom_batch: 20".
> > +
> >  Unknown lines
> >  -------------
> >
> > @@ -206,6 +355,7 @@ An example of a test with two nested subtests:
> >         KTAP version 2
> >         1..1
> >           KTAP version 2
> > +         # ktap_test: example
> >           1..2
> >           ok 1 test_1
> >           not ok 2 test_2
> > @@ -219,6 +369,7 @@ An example format with multiple levels of nested testing:
> >         KTAP version 2
> >         1..2
> >           KTAP version 2
> > +         # ktap_test: example_test_1
> >           1..2
> >             KTAP version 2
> >             1..2
> > @@ -254,6 +405,7 @@ Example KTAP output
> >         KTAP version 2
> >         1..1
> >           KTAP version 2
> > +         # ktap_test: main_test
> >           1..3
> >             KTAP version 2
> >             1..1
> > @@ -261,11 +413,14 @@ Example KTAP output
> >             ok 1 test_1
> >           ok 1 example_test_1
> >             KTAP version 2
> > +        # ktap_test: example_test_2
> > +        # ktap_speed: slow
> >             1..2
> >             ok 1 test_1 # SKIP test_1 skipped
> >             ok 2 test_2
> >           ok 2 example_test_2
> >             KTAP version 2
> > +           # ktap_test: example_test_3
> >             1..3
> >             ok 1 test_1
> >             # test_2: FAIL
> >
> > base-commit: 906f02e42adfbd5ae70d328ee71656ecb602aaf5
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >

Powered by blists - more mailing lists