> -----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. However, I can see how this would be confusing and possibly error prone. 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. 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. -- 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
