Date:   Thu, 7 Oct 2021 14:22:05 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v6] kunit: tool: improve compatibility of kunit_parser
 with KTAP specification

On Thu, Oct 7, 2021 at 1:00 AM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> From: Rae Moar <rmoar@...gle.com>
>
> Update to kunit_parser to improve compatibility with KTAP
> specification including arbitrarily nested tests. Patch accomplishes
> three major changes:
>
> - Use a general Test object to represent all tests rather than TestCase
> and TestSuite objects. This allows for easier implementation of arbitrary
> levels of nested tests and promotes the idea that both test suites and test
> cases are tests.
>
> - Print errors incrementally rather than all at once after the
> parsing finishes to maximize information given to the user in the
> case of the parser given invalid input and to increase the helpfulness
> of the timestamps given during printing. Note that kunit.py parse does
> not print incrementally yet. However, this fix brings us closer to
> this feature.
>
> - Increase compatibility for different formats of input. Arbitrary levels
> of nested tests supported. Also, test cases and test suites are now
> supported to be present on the same level of testing.
>
> This patch now implements the draft KTAP specification here:
> https://lore.kernel.org/linux-kselftest/CA+GJov6tdjvY9x12JsJT14qn6c7NViJxqaJk+r-K1YJzPggFDQ@mail.gmail.com/
> We'll update the parser as the spec evolves.
>
> This patch adjusts the kunit_tool_test.py file to check for
> the correct outputs from the new parser and adds a new test to check
> the parsing for a KTAP result log with correct format for multiple nested
> subtests (test_is_test_passed-all_passed_nested.log).
>
> This patch also alters the kunit_json.py file to allow for arbitrarily
> nested tests.
>
> Signed-off-by: Rae Moar <rmoar@...gle.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@...gle.com>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> Reviewed-by: David Gow <davidgow@...gle.com>
> ---
> NOTE: this patch is now applied on top of
> https://lore.kernel.org/linux-kselftest/20210930222048.1692635-5-dlatypov@google.com/
> to resolve a conflict.
>
> Change log from v5:
> https://lore.kernel.org/linux-kselftest/20211006001447.20919-1-dlatypov@google.com/
> - Tweak commit message to reflect the KTAP spec is a draft
> - Add missing Signed-off-by
> - Tweak docstrings
>
> Change log from v3,4:
> https://lore.kernel.org/linux-kselftest/20210901190623.315736-1-rmoar@google.com/
> - Move test_kselftest_nested from LinuxSourceTreeTest => KUnitParserTest.
> - Resolve conflict with hermetic testing patches.
>   - max_status is no longer defined, so we need to use the TestCounts
>     type now. And to keep --raw_output working, we need to set this to
>     SUCCESS to avoid the default assumption that the kernel crashed.
>
> Ignore v4, was accidentally based on v2.
>
> Change log from v2:
> https://lore.kernel.org/linux-kselftest/20210826195505.3066755-1-rmoar@google.com/
> - Fixes bug of type disagreement in kunit_json.py for build_dir
> - Removes raw_output()
> - Changes docstrings in kunit_parser.py (class docstring, LineStream
>   docstrings, add_error(), total(), get_status(), all parsing methods)
> - Fixes bug of not printing diagnostic log in the case of end of lines
> - Sets default status of all tests to TEST_CRASHED
> - Adds and prints empty tests with crashed status in case of missing
>   tests
> - Prints 'subtest' in instance of 1 subtest instead of 'subtests'
> - Includes checking for 'BUG:' message in search of crash messages in
>   log (note that parse_crash_in_log method could be removed but would
>   require deleting tests in kunit_tool_test.py that include the crash
>   message that is no longer used. If removed, parser would still print
>   log in cases of test crashed or failure, which would now include
>   missing subtests)
> - Fixes bug of including directives (other than SKIP) in test name
>   when matching name in result line for subtests
>
>
> Change log from v1:
> https://lore.kernel.org/linux-kselftest/20210820200032.2178134-1-rmoar@google.com/
> - Rebase onto kselftest/kunit branch
> - Add tests to kunit_tool_test.py to check parser is correctly stripping
>   hyphen, producing correct json objects with nested tests, correctly
>   passing kselftest TAP output, and correctly deals with missing test plan.
> - Fix bug to correctly match test name in instance of a missing test plan.
> - Fix bug in kunit_tool_test.py pointed out by Daniel where it was not
>   correctly checking for a proper match to the '0 tests run!' error
>   message. Reverts changes back to original.
> - A few minor changes to commit message using Daniel's comments.
> - Change docstrings using Daniel's comments to reduce:
>   - Shortens some docstrings to be one-line or just description if it is
>     self explanatory.
>   - Remove explicit respecification of types of parameters and returns
>     because this is already specified in the function annoations. However,
>     some descriptions of the parameters and returns remain and some contain
>     the type for context. Additionally, the types of public attributes of
>     classes remain.
>   - Remove any documentation of 'Return: None'
>   - Remove docstrings of helper methods within other methods
> ---

Looks good to me, thanks!

This is still:
Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David

Powered by blists - more mailing lists