Re: [PATCH v3 1/4] clk: add a clk_hw helpers to get the clock device or device_node

[Stephen Boyd <sboyd@kernel.org>]

Quoting Jerome Brunet (2025-03-21 10:53:49)
> On Wed 26 Feb 2025 at 17:01, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> 
> >> +static void clk_hw_get_of_node_test(struct kunit *test)
> >> +{
> >> +       struct device_node *np;
> >> +       struct clk_hw *hw;
> >> +
> >> +       hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
> >> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> >> +
> >> +       np = of_find_compatible_node(NULL, NULL, "test,clk-dummy-device");
> >> +       hw->init = CLK_HW_INIT_NO_PARENT("test_get_of_node",
> >> +                                        &clk_dummy_rate_ops, 0);
> >> +       of_node_put_kunit(test, np);
> >> +
> >> +       KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, np, hw));
> >
> > The stuff before the expectation should likely go to the init function.
> > Or it can use the genparams stuff so we can set some struct members to
> > indicate if the pointer should be NULL or not and then twist through the
> > code a couple times.
> >
> 
> I'm trying to address all your comments but I'm starting to wonder if
> this isn't going a bit too far ? The functions tested are one line
> returns. Is it really worth all this ?
> 
> I do understand the idea for things that actually do something, such as
> reparenting, setting rates or what not ... But this ? It feels like a
> lot of test code for very little added value, don't you think ?
> 

Just so I understand, you're saying that this is always going to be a
simple "getter" API that doesn't do much else? We're not _only_ testing
the getter API, we're also testing the registration path that actually
sets the device or of_node pointers for a clk. I'm not really thinking
about the one line return functions here.

Writing tests is definitely a balancing act. I'd say we want to test the
behavior of the API in relation to how a clk is registered and writing
tests to show the intended usage is helpful to understand if we've
thought of corner cases like the clk was registered with a device
pointer that also has an of_node associated with it. (Did we remember to
stash that of_node pointer too?) We have a bunch of clk registration
APIs, and we want to make sure this getter API works with all of them.
Note that we don't care about the clk flags or parent relation chains
here, just that the device or of_node passed in to registration comes
out the other side with the getter API.

A little code duplication is OK, as long as the test is easy to
understand. Maybe genparams stuff is going too far, I don't know, but at
the least we want to make sure the clk registration APIs behave as
expected when the getter API is used to get the device or of_node later.

I've found this google chapter[1] useful for unit testing best
practices. I recommend reading it if you haven't already.

[1] https://abseil.io/resources/swe-book/html/ch12.html