> -----Original Message-----
> From: Paul Elder on Wednesday, June 14, 2017 11:59 AM
> On 06/14/2017 05:17 AM, Bird, Timothy wrote:
> >
> >> -----Original Message-----
> >> From: Alice Ferrazzi on Wednesday, June 14, 2017 4:46 AM
> >>
> >> 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");
> >> }
> >
> > I have a few comments on this. First, I hesitate to introduce
> > a consistent schema for message results output for kselftests
> > at this early stage of development.
> > That's why I went with something very simple - just a single line
> > containing an explanation of the failure.
> >
> > I think we should see if there is consistency in the extra output
> > that tests have, after converting several to TAP format, to determine
> > what common schema, if any, is appropriate.
> >
> > If we do end up with a common schema for extra information,
> > then having a helper function for it, as you've shown, would be
> > very nice to help with consistency. And an extended schema
> > should be a superset of a minimal schema, also for consistency.
> >
> > In other words, you'd probably want to have 'reason' in the
> > longer schema, and then you could have three types of failure
> > calls:
> >
> > # just show failure result and test name
> > 1) ksft_fail(test_name, NULL);
> >
> > # show failure result and reason
> > 2) ksft_fail(test_name, "one-line reason for failure");
> I'm thinking that since the "one-line reason for failure" is yaml
> anyway, why don't we make the function for outputting that together
> with the specialized yaml output function?
>
> As you have pointed out, some tests will not output a reason for
> failure, so then they can get the empty ksft_fail() call and then
> if there is a failure reason then they can call ksft_yaml()
> afterwards (with whatever parameters we decide on).
Either way would work.
Whether to add an extra parameter to ksft_fail()
(or ksft_test_result_fail(), since it appears we're not renaming them),
for the single-line YAML case is largely a matter of taste.
It all comes down to frequency of the different scenarios, IMHO.
If the most common operation is to call the 'fail' function with
no extra data, then you don't want to require a NULL parameter
on potentially hundreds of call sites. On the other hand, if it's quite
common to call the 'fail' function with exactly one line of extra data,
then integrating it into a single call is convenient, as opposed
to requiring each call site to call two functions for each result.
IMHO it's quite nice to insulate the test program from the TAP
format. So I think it would be good to have something like
ksft_yaml() - or maybe something more generically named like
ksft_output_extra_data()
I just don't want to lose the extra explanation strings that were
being emitted before. So we should decide on some facility for
outputting them. And I don't think putting the explanation string
on the same line as the test description is a good idea.
-- Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
