On Tue, Jun 13, 2017 at 06:49:22PM +0000, Bird, Timothy wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman on Monday, June 12, 2017 3:57 PM
> >
> > Add TAP13 conformat output functions to kselftest.h.
> >
> > Also add exit functions that output TAP13 exiting text, as well as
> > functions to keep track of testing progress.
> >
> > Signed-off-by: Paul Elder <paul.elder@xxxxxxxx>
> > Signed-off-by: Alice Ferrazzi <alice.ferrazzi@xxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> > v2: Just use the standard function names, no _tap suffix - Alice
> >
> > tools/testing/selftests/kselftest.h | 52
> > ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 48 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kselftest.h
> > b/tools/testing/selftests/kselftest.h
> > index ef1c80d67ac7..1d874a50d957 100644
> > --- a/tools/testing/selftests/kselftest.h
> > +++ b/tools/testing/selftests/kselftest.h
> > @@ -31,38 +31,82 @@ struct ksft_count {
> >
> > static struct ksft_count ksft_cnt;
> >
> > +static inline int ksft_test_num(void)
> > +{
> > + return ksft_cnt.ksft_pass + ksft_cnt.ksft_fail +
> > + ksft_cnt.ksft_xfail + ksft_cnt.ksft_xpass +
> > + ksft_cnt.ksft_xskip;
> > +}
> > +
> > static inline void ksft_inc_pass_cnt(void) { ksft_cnt.ksft_pass++; }
> > static inline void ksft_inc_fail_cnt(void) { ksft_cnt.ksft_fail++; }
> > static inline void ksft_inc_xfail_cnt(void) { ksft_cnt.ksft_xfail++; }
> > static inline void ksft_inc_xpass_cnt(void) { ksft_cnt.ksft_xpass++; }
> > static inline void ksft_inc_xskip_cnt(void) { ksft_cnt.ksft_xskip++; }
> >
> > +static inline void ksft_print_header(void)
> > +{
> > + printf("TAP version 13\n");
> > +}
> > +
> > static inline void ksft_print_cnts(void)
> > {
> > - printf("Pass: %d Fail: %d Xfail: %d Xpass: %d, Xskip: %d\n",
> > - ksft_cnt.ksft_pass, ksft_cnt.ksft_fail,
> > - ksft_cnt.ksft_xfail, ksft_cnt.ksft_xpass,
> > - ksft_cnt.ksft_xskip);
> > + printf("1..%d\n", ksft_test_num());
> > +}
> > +
> > +static inline void ksft_test_result_pass(const char *msg)
>
> IMHO something shorter like 'ksft_pass' would be better. However, that's
> a variable name in struct ksft_count. Technically there would be no conflict
> to have both a function and a structure variable with the same name, but it
> would be confusing to maintain. I would recommend using the shorter
> name for the functions, and renaming the internal variables, as shown
> below.
>
> I also think it's not critical to prefix every variable with ksft_ in this structure.
> The variables are only ever accessed using the variable ksft_cnt, so the extra
> prefix is not needed.
>
> struct ksft_count {
> unsigned int pass_count;
> unsigned int fail_count;
> unsigned int xfail_count;
> unsigned int xpass_count;
> unsigned int xskip_count;
> };
>
> Of course this rename has to flow through all references, like so:
>
> > +{
> > + ksft_cnt.ksft_pass++;
> ksft_cnt.pass_count++;
>
> > + printf("ok %d %s\n", ksft_test_num(), msg);
> > +}
> > +
> > +static inline void ksft_test_result_fail(const char *msg)
> > +{
> > + ksft_cnt.ksft_fail++;
> > + printf("not ok %d %s\n", ksft_test_num(), msg);
> > +}
>
> Same thing here on function name length. Maybe 'kfst_fail'?
>
> I think that for all these reporting functions, the first parameters
> should be const char *desc. It should be the same whether the
> test passes or fails. Then on a fail, there should be an optional
> additional parameter indicating the reason for the failure.
>
> It's important that the test description be invariant between runs.
> This can be used as the human-readable identifier for each test.
> You don't want this message to be a test description on success, but
> a reason for failure on error. That's not consistent.
>
> On success, no other string is required. On failure, it is nice (but optional)
> to output the reasons for the failure. I would recommend doing this in a
> YAML block, like so:
>
> static inline void ksft_fail(const char *desc, const char *reason)
> {
> ksft_cnt.ksft_pass++;
> printf("ok %d %s\n", ksft_test_num(), desc);
> if (reason)
> printf(" ---\n reason: \"%s\"\n ...\n", reason);
> }
From TAP 13, YAML blocks, will retrive a data structure with information
about the test.
From the TAP 13 example there is a:
message
severity
data
Maybe just having a reason can be not enough when there is also some data to show ?
Having also a severity and data would maybe be better ?
static inline int ksft_yaml(const char *msg, const char *severity,
const char *data)
{
printf("---\n");
printf("message: %s \n", msg);
printf("severity: %s \n", severity);
printf("data: %s \n", data);
printf("...\n");
}
>
> Note the leading space for each line of the YAML block, to indent it according
> to the TAP13 protocol.
>
> Then a call is either:
> ksft_fail(desc, NULL);
> or
> ksft_fail(desc, "foo index failure")
>
> > +
> > +static inline void ksft_test_result_skip(const char *msg)
> > +{
> > + ksft_cnt.ksft_xskip++;
> > + printf("ok %d # skip %s\n", ksft_test_num(), msg);
> > }
>
> You should add an optional 'reason' for ksft_skip as well.
>
> > static inline int ksft_exit_pass(void)
> > {
> > + ksft_print_cnts();
> > exit(KSFT_PASS);
> > }
> > +
> > static inline int ksft_exit_fail(void)
> > {
> > + printf("Bail out!\n");
> > + ksft_print_cnts();
> > exit(KSFT_FAIL);
> > }
> > +
> > +static inline int ksft_exit_fail_msg(const char *msg)
> > +{
> > + printf("Bail out! %s\n", msg);
> > + ksft_print_cnts();
> > + exit(KSFT_FAIL);
> > +}
> > +
> > static inline int ksft_exit_xfail(void)
> > {
> > + ksft_print_cnts();
> > exit(KSFT_XFAIL);
> > }
> > +
> > static inline int ksft_exit_xpass(void)
> > {
> > + ksft_print_cnts();
> > exit(KSFT_XPASS);
> > }
> > +
> > static inline int ksft_exit_skip(void)
> > {
> > + ksft_print_cnts();
> > exit(KSFT_SKIP);
> > }
> >
> > --
> > 2.13.1
> >
>
Attachment:
signature.asc
Description: PGP signature
