Message-ID: <f12ee323-ff6f-b4c9-02d8-e0b4a46e74fc@redhat.com>
Date:   Fri, 9 Jun 2023 12:35:48 +0200
From:   Marco Pagani <marpagan@...hat.com>
To:     Xu Yilun <yilun.xu@...el.com>
Cc:     Moritz Fischer <mdf@...nel.org>, Wu Hao <hao.wu@...el.com>,
        Tom Rix <trix@...hat.com>, linux-kernel@...r.kernel.org,
        linux-fpga@...r.kernel.org
Subject: Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA
 Region



On 2023-06-09 13:09, Xu Yilun wrote:
> On 2023-06-07 at 17:57:22 +0200, Marco Pagani wrote:
>>
>>
>> On 2023-06-06 13:00, Xu Yilun wrote:
>>> On 2023-06-05 at 18:58:56 +0200, Marco Pagani wrote:
>>>>
>>>>
>>>> On 2023-06-03 21:11, Xu Yilun wrote:
>>>>> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
>>>>>> The suite tests the programming of an FPGA Region with a Bridge
>>>>>> and the function for finding a particular Region.
>>>>>>
>>>>>> Signed-off-by: Marco Pagani <marpagan@...hat.com>
>>>>>> ---
>>>>>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>>>>>>  1 file changed, 186 insertions(+)
>>>>>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
>>>>
>>>> [...]
>>>>
>>>>  
>>>>> Maybe better just put all tests in one module, and have unified
>>>>> fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
>>>>>
>>>>> In previous thread, I said I'm good to the self-contained test module
>>>>> but I didn't actually follow the idea. Sorry for that.
>>>>>
>>>>> The concern is why in this region test, the write_count and only the
>>>>> write_count is taken care of.
>>>>>
>>>>> Although fpga_mgr_load() test covers all mgr_ops, but does that
>>>>> means these ops are still good for more complex case like
>>>>> fpga_region_program_fpga()? And there is no guarantee
>>>>> fpga_region_program_fpga() would always call mgr_ops the same way
>>>>> as fpga_mgr_load() in future.
>>>>>
>>>>> Similar for fpga_bridge. Maybe a complete setup for fpga_region is
>>>>> still necessary.
>>>>
>>>> I think that putting all tests in a single module (like in previous
>>>> versions) goes against the principles of unit testing, making the
>>>> code more similar to an integration test.
>>>>
>>>> Unit tests should be focused on a single behavior. The programming
>>>> test case included in the Region's suite should test only the behavior
>>>> of the Region itself. Specifically, that fpga_region_program_fpga() calls
>>>> get_bridges(), to get and control bridges, and then the Manager for the
>>>> actual programming.
>>>>
>>>> The programming sequence itself is outside the responsibilities of the
>>>> Region, and its correctness is already ensured by the Manager suite.
>>>> Similarly, the correctness of the Bridge's methods used by the Region
>>>> for getting and controlling multiple bridges is already ensured by the
>>>> Bridge test suite.
>>>>
>>>> For this reason, the Manager and Bridge fakes used in the Region suite
>>>> implement only the minimal set of operations necessary to ensure the
>>>> correctness of the Region's behavior. If I used a "full" Manager (and
>>>> tested all mgr_ops), then the test case would have become an integration
>>>> test rather than a unit test for the Region.
>>>
>>> I agree with you about a unit test should focus on a single behavior. But
>>> I have concerns that each test suite uses different definitions of the
>>> same structure, mgr/bridge stats, mgr/bridge ops, mgr/bridge ctx. Even
>>> if we have full definitions for these structures to acommodate all
>>> tests, it doesn't break the principle of unit test, just ignore the fields
>>> and skip checks that you don't care. E.g. only checks mgr.write_count &
>>> bridge.enable_count for region test.
>>>
>>> And a single module simplifies the implementation.
>>>
>>> struct mgr_stats {
>>> 	...
>>> };
>>>
>>> struct mgr_ctx {
>>> 	struct fpga_image_info *img_info;
>>> 	struct fpga_manager *mgr;
>>> 	struct platform_device *pdev;
>>> 	struct mgr_stats stats;
>>> };
>>>
>>> struct bridge_stats {
>>> 	...
>>> };
>>>
>>> struct bridge_ctx {
>>> 	struct fpga_bridge *bridge;
>>> 	struct platform_device *pdev;
>>> 	struct bridge_stats stats;
>>> };
>>>
>>> struct region_ctx {
>>> 	struct mgr_ctx mgr_ctx;
>>> 	struct bridge_ctx bridge_ctx;
>>>
>>> 	struct fpga_region *region;
>>> 	struct platform_device *region_pdev;
>>> };
>>>
>>> How do you think?
>>>
>>> Thanks,
>>> Yilun
>>>
>>
>> My concern with unified fakes having the same ops, stats, and context structs
>> is that they would couple the test suites together. I think it's better to
>> have multiple fakes, each with the single responsibility of providing minimal
>> support for the component under test. Otherwise, we would end up having
>> overcomplicated fakes that implement the union (in the set theory sense of
>> the term) of all behaviors tested by all suites. By using these fakes, some
>> test cases might implicitly exercise behaviors that are outside their scope
>> (e.g., the Region programming test case calling all Manager ops). I feel
>> this would go against the principle of limiting the amount of code under test
>> to a single unit. 
> 
> OK. On second thought, it is good to me.
> 
> I think now the high level opens are all addressed. Will you keep on
> improving the patchset or make it stable for upstream? If the later, you
> may drop the RFC prefix.

I plan to stabilize the patchset for the upstream.

Thanks,
Marco

> 
> Thanks,
> Yilun
> 
>> Thanks,
>> Marco
>>
>>>>> BTW: I like the way that fake drivers are removed. Looks much straight
>>>>> forward.
>>>>
>>>> I appreciate that.
>>>>  
>>>>> Thanks,
>>>>> Yilun
>>>>>
>>>>
>>>> Thanks,
>>>> Marco
>>>>
>>>> [...]
>>>>
>>>
>>
> 

Powered by blists - more mailing lists