On Mon, Sep 28, 2020 at 4:24 PM Brendan Higgins brendanhiggins@google.com wrote:
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-r...
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.
It works with this code as-is, yes.
But messing around with it, it seems like it might be helpful for the mock init funcs access to `struct kunit *test` in case they want to allocate.
E.g. in cases where the ops struct points to a shared instance. We'd want to allocate a new ops struct so we don't accidentally affect more objects than expected by altering global state.
It's a simple enough change, i.e.
diff --git a/include/kunit/mock.h b/include/kunit/mock.h index 955c4267d726..9006f0e492dc 100644 --- a/include/kunit/mock.h +++ b/include/kunit/mock.h @@ -613,7 +613,7 @@ static inline bool is_naggy_mock(struct mock *mock) \ mock_init_ctrl(test, mock_get_ctrl(mock_obj)); \ \ - if (init_func(mock_obj)) \ + if (init_func(test, mock_obj)) \ return NULL; \ \ return mock_obj; \ diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c index cd538cdb2a96..38c87f163d2f 100644 --- a/lib/kunit/kunit-example-test.c +++ b/lib/kunit/kunit-example-test.c @@ -66,7 +66,7 @@ DEFINE_STRUCT_CLASS_MOCK(METHOD(foo), CLASS(example), * parent. In this example, all we have to do is populate the member functions * of the parent class with the mock versions we defined. */ -static int example_init(struct MOCK(example) *mock_example) +static int example_init(struct kunit *test, struct MOCK(example) *mock_example) { /* This is how you get a pointer to the parent class of a mock. */ struct example *example = mock_get_trgt(mock_example);
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()).
Yeah, I'm still not sure about this.
One approach would be to only support objects we can allocate and thus embed inside a wrapper MOCK(class) struct. Users would have to write a custom wrapper struct and from_<class>_to_mock() funcs for everything else.
E.g. they'd write
struct MOCK(pci_bus) { struct mock ctrl; struct pci_bus *bus; // normally, the macro would generate without * }
static inline struct mock *from_pci_ops_to_mock( const struct pci_bus *self) { // let user provide magic logic to do this lookup struct MOCK(pci_bus) *mock = somehow_get_wrapper(pci_bus); return mock_get_ctrl(mock); }
With the proposed change above to pass a `struct kunit *` to the init, we open the possibility of using a kunit_resource to store this mapping. Perhaps the conversion func could also be changed to take a `struct kunit *` as well?
static int mock_pci_bus_init(struct kunit *test, struct MOCK(pci_bus) *mocked) { // somehow establish mapping kunit_add_named_resource(test, ....); }
Given resources are looked up via a linear scan, perhaps we do something like create a single instance for each mocked type. E.g. we have a `map_pci_bus_to_mock` hashtable that either gets allocated or updated with each call?
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()).
Here's the function pointers void (*ss_cb) (bool assert, struct cfspi_ifc *ifc); void (*xfer_done_cb) (struct cfspi_ifc *ifc);
So sadly that won't work (unless you only wanted to mock one of the funcs). But as we agree that we don't want to try and support everything, this is fine.
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.
Agree. The code itself is more or less able to handle most of these use cases. Concern was more about how frequent + painful minor deviations from the current pattern would be. As noted above, I think allowing the mock init funcs to safely allocate a new ops struct if they want might be enough.
For more exotic cases, users can write custom from_<class>_to_mock() funcs to handle a lot of cases. As long as the first argument to the mocked functions matches the first argument of _to_mock(), then it should all work out.
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.
[...]