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

[Felix Maurer <fmaurer@redhat.com>]

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.
>>
>> [1]: https://github.com/linux-can/can-tests
>>
>> 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