Re: [PATCH v2] selftests: can: Import tst-filter from can-tests

Hi Vincent,

On 14.05.25 11:47, Vincent Mailhol wrote:

Hi Felix,

On Sat. 10 May 2025 at 00:07, Felix Maurer [off-list ref] wrote:

quoted

Tests for the can subsystem have been in the can-tests repository[1] so
far. Start moving the tests to kernel selftests by importing the current
tst-filter test. The test is now named test_raw_filter and is substantially
updated to be more aligned with the kernel selftests, follow the coding
style, and simplify the validation of received CAN frames. We also include
documentation of the test design. The test verifies that the single filters
on raw CAN sockets work as expected.

We intend to import more tests from can-tests and add additional test cases
in the future. The goal of moving the CAN selftests into the tree is to
align the tests more closely with the kernel, improve testing of CAN in
general, and to simplify running the tests automatically in the various
kernel CI systems.

[1]: https://github.com/linux-can/can-tests

Signed-off-by: Felix Maurer <redacted>

Thanks again.

I left a set of nitpicks, I expect to give my reviewed-by tag on the
next version.

Thank you for your feedback. I'll post a new version with the changes
included soon.

[...]

quoted

+FIXTURE_SETUP(can_filters)
+{
+       struct sockaddr_can addr;
+       struct ifreq ifr;
+       int recv_own_msgs = 1;
+       int s, ret;
+
+       s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
+       ASSERT_LT(0, s)

0 is a valid fd (OK it is used for the stout, so your code will work,
but the comparison still looks unnatural).

What about:

  ASSERT_NE(s, -1)

or:

  ASSERT_GE(s, 0)

?

(same comment for the other ASSERT_LE)

I was a bit hesitant to change the order of expected and seen value for
the the assertions because it's documented as ASSERT_*(expected, seen).
But it seems to be common in the selftest to not follow this order where
assertions are used for error checking and failure message doesn't
explicitly say what was expected/seen. I'll take a look at the error
checking in the whole file where the more familiar form is with reversed
arguments.

Thanks,
   Felix