Messages in this thread | | | From | Brendan Higgins <> | Date | Mon, 28 Sep 2020 16:24:25 -0700 | Subject | Re: [RFC v1 00/12] kunit: introduce class mocking support. |
| |
On Tue, Sep 22, 2020 at 5:24 PM Daniel Latypov <dlatypov@google.com> wrote: > > On Fri, Sep 18, 2020 at 11:31 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > # Background > > KUnit currently lacks any first-class support for mocking. > > For an overview and discussion on the pros and cons, see > > https://martinfowler.com/articles/mocksArentStubs.html > > > > This patch set introduces the basic machinery needed for mocking: > > setting and validating expectations, setting default actions, etc. > > > > Using that basic infrastructure, we add macros for "class mocking", as > > it's probably the easiest type of mocking to start with. > > > > ## Class mocking > > > > By "class mocking", we're referring mocking out function pointers stored > > in structs like: > > struct sender { > > int (*send)(struct sender *sender, int data); > > }; > > Discussed this offline a bit with Brendan and David. > The requirement that the first argument is a `sender *` means this > doesn't work for a few common cases. > > E.g. ops structs don't usually have funcs that take `XXX_ops *` > `pci_ops` all take a `struct pci_bus *`, which at least contains a > `struct pci_ops*`. > > Why does this matter? > We need to stash a `struct mock` somewhere to store expectations. > For this version of class mocking (in patch 10), we've assumed we could have > > struct MOCK(sender) { > struct mock ctrl; > struct sender trgt; //&trgt assumed to be the first param > } > > Then we can always use `container_of(sender)` to get the MOCK(sender) > which holds `ctrl`. > But if the first parameter isn't a `struct sender*`, this trick > obviously doesn't work. > > So to support something like pci_ops, we'd need another approach to > stash `ctrl`. > > After thinking a bit more, we could perhaps decouple the first param > from the mocked struct. > Using pci_ops as the example: > > struct MOCK(pci_ops) { > struct mock ctrl; > struct pci_bus *self; // this is the first param. Using > python terminology here. > struct pci_ops trgt; // continue to store this, as this holds > the function pointers > } > > // Name of this func is currently based on the class we're mocking > static inline struct mock *from_pci_ops_to_mock( > const struct pci_bus *self) > { > return mock_get_ctrl(container_of(self, struct MOCK(pci_ops), self)); > } > > // then in tests, we'd need something like > struct pci_bus bus; > bus.ops = mock_get_trgt(mock_ops); > mock_ops.self = &bus; > > Do others have thoughts/opinions?
In the above example, wouldn't we actually want to mock struct pci_bus, not struct pci_ops?
I think pci_bus is what actually gets implemented. Consider the Rockchip PCIe host controller:
Rockchip provides it's own pci_ops struct:
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pcie-rockchip-host.c#L256
If you look at one of the implemented methods, say rockchip_pcie_rd_conf(), you will see:
... struct rockchip_pcie *rockchip = bus->sysdata; ... (This function signature is:
int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
).
Thus, conceptually struct pci_bus is what is being manipulated; it best fits the candidate for the *self pointer.
In fact - if I am not mistaken - I think if you were to mock pci_bus and just pretend that the methods were on pci_bus, not pci_ops, I think it might just work.
The bigger problem is that it seems pci_bus does not want the user to allocate it: in the case of rockchip_pcie, it is allocated by devm_pci_alloc_host_bridge(). Thus, embedding pci_bus inside of a MOCK(pci_bus) would probably not work, so you would need to have different tooling around that and would then need to provide a custom definition of from_pci_bus_to_mock() (generated by DECLARE_STRUCT_CLASS_MOCK_CONVERTER()).
> After grepping around for examples, I'm struck by how the number of > outliers there are. > E.g. most take a `struct thing *` as the first param, but cfspi_ifc in > caif_spi.h opts to take that as the last parameter.
That's not a problem, just use the DECLARE_STRUCT_CLASS_MOCK_HANDLE_INDEX() variant to create your mock (as opposed to DECLARE_STRUCT_CLASS_MOCK()).
For example let's say you have the following struct that you want to mock:
struct example { ... int (*foo)(int bar, char baz, struct example *self); };
You could mock the function with:
DECLARE_STRUCT_CLASS_MOCK_HANDLE_INDEX( METHOD(foo), CLASS(example), 2, /* The "handle" is in position 2. */ RETURNS(int), PARAMS(int, char, struct example *));
> And others take a different mix of structs for each function. > > But it feels like a decoupled self / struct-holding-func-pointers > approach should be generic enough, as far as I can tell. > The more exotic types will probably have to remain unsupported.
I think the code is pretty much here to handle any situation in which one of the parameters input to the function can be used to look up a mock object; we just need to expose the from_<class>_to_mock() function to the user to override. The problem I see with what we have now is the assumption that the user gets to create the object that they want to mock. Your example has illustrated that that is not a safe assumption.
But yes, sufficiently exoctic cases will have to be unsupported.
BTW, sorry for not sharing the above in our offline discussion; since we were talking without concrete examples in front of us, the problem sounded worse than it looks here.
[...]
| |