Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210528075932.347154-1-davidgow@google.com>
In-Reply-To: <20210528075932.347154-1-davidgow@google.com>
From:   Brendan Higgins <brendanhiggins@google.com>
Date:   Fri, 4 Jun 2021 14:04:45 -0700
Message-ID: <CAFd5g44iCQtQ0XqDsKgQaVu=c2hq0NXbsqquEaQpRFEqnAMyFA@mail.gmail.com>
Subject: Re: [PATCH v2 1/4] kunit: Support skipped tests
To:     David Gow <davidgow@google.com>
Cc:     Alan Maguire <alan.maguire@oracle.com>,
        Daniel Latypov <dlatypov@google.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Marco Elver <elver@google.com>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, May 28, 2021 at 12:59 AM David Gow <davidgow@google.com> wrote:
>
> The kunit_mark_skipped() macro marks the current test as "skipped", with
> the provided reason. The kunit_skip() macro will mark the test as
> skipped, and abort the test.
>
> The TAP specification supports this "SKIP directive" as a comment after
> the "ok" / "not ok" for a test. See the "Directives" section of the TAP
> spec for details:
> https://testanything.org/tap-specification.html#directives
>
> The 'success' field for KUnit tests is replaced with a kunit_status
> enum, which can be SUCCESS, FAILURE, or SKIPPED, combined with a
> 'status_comment' containing information on why a test was skipped.
>
> A new 'kunit_status' test suite is added to test this.
>
> Signed-off-by: David Gow <davidgow@google.com>
> Tested-by: Marco Elver <elver@google.com>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>

One fairly minor nit below. Other than that, looks great!

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---

[...]

> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index b68c61348121..1401c620ac5e 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -105,6 +105,18 @@ struct kunit;
>  #define KUNIT_SUBTEST_INDENT           "    "
>  #define KUNIT_SUBSUBTEST_INDENT                "        "
>
> +/**
> + * enum kunit_status - Type of result for a test or test suite
> + * @KUNIT_SUCCESS: Denotes the test suite has not failed nor been skipped
> + * @KUNIT_FAILURE: Denotes the test has failed.
> + * @KUNIT_SKIPPED: Denotes the test has been skipped.
> + */
> +enum kunit_status {
> +       KUNIT_SUCCESS,
> +       KUNIT_FAILURE,
> +       KUNIT_SKIPPED,
> +};
> +
>  /**
>   * struct kunit_case - represents an individual test case.
>   *
> @@ -148,13 +160,20 @@ struct kunit_case {
>         const void* (*generate_params)(const void *prev, char *desc);
>
>         /* private: internal use only. */
> -       bool success;
> +       enum kunit_status status;
>         char *log;
>  };
>
> -static inline char *kunit_status_to_string(bool status)
> +static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
>  {
> -       return status ? "ok" : "not ok";
> +       switch (status) {
> +       case KUNIT_SKIPPED:
> +       case KUNIT_SUCCESS:
> +               return "ok";
> +       case KUNIT_FAILURE:
> +               return "not ok";
> +       }
> +       return "invalid";
>  }
>
>  /**
> @@ -212,6 +231,7 @@ struct kunit_suite {
>         struct kunit_case *test_cases;
>
>         /* private: internal use only */
> +       char status_comment[256];

nit: How about we make the 256 a constant since you use it in a number
of places?

If not, at least when you reference the struct, you might want to use
ARRAY_SIZE(...).

>         struct dentry *debugfs;
>         char *log;
>  };
[...]