Hi Vincent,
On 14.05.25 11:47, Vincent Mailhol wrote:
> Hi Felix,
>
> On Sat. 10 May 2025 at 00:07, Felix Maurer <[email protected]> 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.
>>
>> [1]: https://github.com/linux-can/can-tests
>>
>> Signed-off-by: Felix Maurer <[email protected]>
>
> 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
