Hi Vincent,
On 14.05.25 11:47, Vincent Mailhol wrote:
Hi Felix,
On Sat. 10 May 2025 at 00:07, Felix Maurer fmaurer@redhat.com wrote:
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.
Signed-off-by: Felix Maurer fmaurer@redhat.com
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.
[...]
+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