On 06/15/2017 03:53 AM, Bird, Timothy wrote: >> -----Original Message----- >> From: Paul Elder on Wednesday, June 14, 2017 12:12 PM >> On 06/14/2017 04:00 AM, Bird, Timothy wrote: >>>> -----Original Message----- >>>> From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] >>>> Sent: Monday, June 12, 2017 3:57 PM >>>> To: shuah@xxxxxxxxxx >>>> Cc: Paul Elder <paul.elder@xxxxxxxx>; linux-kselftest@xxxxxxxxxxxxxxx; >> Greg >>>> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Bird, Timothy >>>> <Tim.Bird@xxxxxxxx>; Alice Ferrazzi <alice.ferrazzi@xxxxxxxxx> >>>> Subject: [PATCH v2 2/4] kselftest: membarrier: convert to TAP13 output >>>> >>>> From: Paul Elder <paul.elder@xxxxxxxx> >>>> >>>> Make the membarrier test output in the TAP13 format by using the >>>> TAP13 output functions defined in kselftest.h >>>> >>>> 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 >>>> >>>> >>>> .../testing/selftests/membarrier/membarrier_test.c | 35 >> ++++++++++++---- >>>> ------ >>>> 1 file changed, 19 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c >>>> b/tools/testing/selftests/membarrier/membarrier_test.c >>>> index 535f0fef4d0b..cae8c984dfb0 100644 >>>> --- a/tools/testing/selftests/membarrier/membarrier_test.c >>>> +++ b/tools/testing/selftests/membarrier/membarrier_test.c >>>> @@ -21,36 +21,42 @@ static int sys_membarrier(int cmd, int flags) >>>> static enum test_membarrier_status test_membarrier_cmd_fail(void) >>>> { >>>> int cmd = -1, flags = 0; >>>> + const char *test_name = "membarrier command fail"; >>>> >>>> if (sys_membarrier(cmd, flags) != -1) { >>>> - printf("membarrier: Wrong command should fail but >>>> passed.\n"); >>>> + ksft_test_result_fail(test_name); >>> >>> As written, the conversion to TAP loses this explanation of the error. >>> >>> If you follow my recommendations for patch 1/4, then this becomes: >>> ksft_fail(test_name, "membarrier: Wrong command should fail but >> passed.") >> Looking at the TAP 13 specification, I can see your point: where the test >> description is invariant whereas the reason for failure should be printed >> separately. However, as I have pointed out in my reply for patch 1/4, I >> think that the "reason" output should be outputted with the yaml. I do see >> that the failure reason should be tied to the test, but then wouldn't the >> yaml closing become messy if the test wants to output more yaml after the >> failure reason? > > Yes. That's why I showed 3 different examples of usage: > > 1) failure with no extra data: > kfst_test_result_fail(test_name, NULL); > > 2) failure with one line of extra data: > kfst_test_result_fail(test_name, "reason string"); > Note this puts the reason string into YAML block with only > one line of content. > > 3) failure with more lines of extra data: > ksft_test_result_fail(test_name); > ksft_yaml(more data - format TBD); > > My intent was that a caller would use only one of these, and not call > ksft_yaml() if they had already included extra data in the kfst_test_result_fail() call. I think that is good, but two of the three scenarios that you have given don't have the reason string included in the ksft_test_result_fail() call. > However, I can see how this would be confusing and possibly error prone. Indeed. > I don't know if TAP allows multiple yaml blocks, but I would assume not. > > So maybe your idea to just always put extra data in a separate call > is better. Thank you. > > IMHO we can always do something that works now, and loses no existing > information. And if it looks like there are some efficiencies to be gained > from combining operations into fewer function calls because of high-frequency > patterns in the calling code, then we can come back and refactor that later. Sounds good. Thanks, Paul > -- Tim > > [snipped] > -- 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
