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).
Paul
> # show failure result and extended data
> 3) ksft_fail(test_name, NULL);
> ksft_yaml(reason, severity, data);
>
> (with ksft_yaml outputting 'reason: ...' instead of 'message: ...')
>
> And a test could also output a full YAML block itself, if it wanted
> to, if it needs to show some other type of data besides that
> expressed by the common output schema.
>
> Note that I'm not particularly tied to the word "reason", and this could
> be changed to the more generic "message". However, I think in
> most cases when reporting a failure the extra message will likely
> be the reason for the failure, as observed by the test program.
>
> Finally, you need to indent the YAML block. So each of the lines
> in ksft_yaml should have a leading space.
>
>>>
>>> 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
>>>>
>>>
--
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
