On Wed, Feb 14, 2024 at 04:25:14PM -0800, Jakub Kicinski wrote:
On Wed, 14 Feb 2024 22:46:46 +0100 Jakub Sitnicki wrote:
On second thought, if I can suggest a follow up change so this:
ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
... becomes this
ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
You see, we parse test results if they are in TAP format. Lack of test name for xfail'ed and skip'ed tests makes it difficult to report in CI which subtest was it. Happy to contribute it, once this series gets applied.
Should have said "harder", not "difficult". That was an overstatement.
Test name can be extracted from diagnostic lines preceeding the status.
# RUN ip_local_port_range.ip4_stcp.late_bind ... # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT # OK ip_local_port_range.ip4_stcp.late_bind ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
It just makes the TAP parser easier if the test name is included on the status line. That would be the motivation here. Let me know what you think.
Good catch, I just copied what we do for skip and completely missed this. As you said we'd report:
ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
and I think that's sort of closer to valid TAP than to valid KTAP which always mentions test/test_case_name:
https://docs.kernel.org/dev-tools/ktap.html
We currently do the same thing for SKIP, e.g.:
# RUN ip_local_port_range.ip4_stcp.late_bind ... # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT # OK ip_local_port_range.ip4_stcp.late_bind ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
I'm not sure if we can realistically do surgery on the existing print helpers to add the test_name, because:
$ git grep 'ksft_test_result_*' | wc -l 915
That'd be a cruel patch to send.
But I do agree that adding the test_name to the prototype is a good move, to avoid others making the same mistake. Should we introduce a new set of helpers which take the extra arg and call them ksft_test_report_*() instead of ksft_test_result_*() ?
Maybe we're overthinking and a local fix in the harness is enough.
Kees, WDYT?
Yeah, let's separate this fix-up from the addition of the XFAIL logic.