On Tue, Sep 22, 2020 at 5:24 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote: > > On Fri, Sep 18, 2020 at 11:31 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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. [...]
