Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 19:00:52 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Marco Pagani <marpagan@redhat.com>
Cc:     Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        linux-fpga@vger.kernel.org
Subject: Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA
 Region
Message-ID: <ZH8R5APukuZVkIv5@yilunxu-OptiPlex-7050>
References: <20230531095405.342080-1-marpagan@redhat.com>
 <20230531095405.342080-4-marpagan@redhat.com>
 <ZHuQc7WfN1zKOeTE@yilunxu-OptiPlex-7050>
 <d1ef2f9a-f416-e7d6-7481-d81c1941702e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d1ef2f9a-f416-e7d6-7481-d81c1941702e@redhat.com>
Precedence: bulk

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@redhat.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

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