| Message ID | 20200918183114.2571146-1-dlatypov@google.com |
|---|---|
| Headers | show |
| Series | kunit: introduce class mocking support. | expand |
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? 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. 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. > > After the necessary DEFINE_* macros, we can then write code like > struct MOCK(sender) mock_sender = CONSTRUCT_MOCK(sender, test); > > /* Fake an error for a specific input. */ > handle = KUNIT_EXPECT_CALL(send(<omitted>, kunit_int_eq(42))); > handle->action = kunit_int_return(test, -EINVAL); > > /* Pass the mocked object to some code under test. */ > KUNIT_EXPECT_EQ(test, -EINVAL, send_message(...)); > > I.e. the goal is to make it easier to test > 1) with less dependencies (we don't need to setup a real `sender`) > 2) unusual/error conditions more easily. > > In the future, we hope to build upon this to support mocking in more > contexts, e.g. standalone funcs, etc. > > # TODOs > > ## Naming > This introduces a number of new macros for dealing with mocks, > e.g: > DEFINE_STRUCT_CLASS_MOCK(METHOD(foo), CLASS(example), > RETURNS(int), > PARAMS(struct example *, int)); > ... > KUNIT_EXPECT_CALL(foo(mock_get_ctrl(mock_example), ...); > For consistency, we could prefix everything with KUNIT, e.g. > `KUNIT_DEFINE_STRUCT_CLASS_MOCK` and `kunit_mock_get_ctrl`, but it feels > like the names might be long enough that they would hinder readability. > > ## Usage > For now the only use of class mocking is in kunit-example-test.c > As part of changing this from an RFC to a real patch set, we're hoping > to include at least one example. > > Pointers to bits of code where this would be useful that aren't too > hairy would be appreciated. > E.g. could easily add a test for tools/perf/ui/progress.h, e.g. that > ui_progress__init() calls ui_progress_ops.init(), but that likely isn't > useful to anyone. > > > Brendan Higgins (9): > kunit: test: add kunit_stream a std::stream like logger > kunit: test: add concept of post conditions > checkpatch: add support for struct MOCK(foo) syntax > kunit: mock: add parameter list manipulation macros > kunit: mock: add internal mock infrastructure > kunit: mock: add basic matchers and actions > kunit: mock: add class mocking support > kunit: mock: add struct param matcher > kunit: mock: implement nice, strict and naggy mock distinctions > > Daniel Latypov (2): > Revert "kunit: move string-stream.h to lib/kunit" > kunit: expose kunit_set_failure() for use by mocking > > Marcelo Schmitt (1): > kunit: mock: add macro machinery to pick correct format args > > include/kunit/assert.h | 3 +- > include/kunit/kunit-stream.h | 94 +++ > include/kunit/mock.h | 902 +++++++++++++++++++++++++ > include/kunit/params.h | 305 +++++++++ > {lib => include}/kunit/string-stream.h | 2 + > include/kunit/test.h | 9 + > lib/kunit/Makefile | 9 +- > lib/kunit/assert.c | 2 - > lib/kunit/common-mocks.c | 409 +++++++++++ > lib/kunit/kunit-example-test.c | 90 +++ > lib/kunit/kunit-stream.c | 110 +++ > lib/kunit/mock-macro-test.c | 241 +++++++ > lib/kunit/mock-test.c | 531 +++++++++++++++ > lib/kunit/mock.c | 370 ++++++++++ > lib/kunit/string-stream-test.c | 3 +- > lib/kunit/string-stream.c | 5 +- > lib/kunit/test.c | 15 +- > scripts/checkpatch.pl | 4 + > 18 files changed, 3091 insertions(+), 13 deletions(-) > create mode 100644 include/kunit/kunit-stream.h > create mode 100644 include/kunit/mock.h > create mode 100644 include/kunit/params.h > rename {lib => include}/kunit/string-stream.h (95%) > create mode 100644 lib/kunit/common-mocks.c > create mode 100644 lib/kunit/kunit-stream.c > create mode 100644 lib/kunit/mock-macro-test.c > create mode 100644 lib/kunit/mock-test.c > create mode 100644 lib/kunit/mock.c > > > base-commit: 10b82d5176488acee2820e5a2cf0f2ec5c3488b6 > -- > 2.28.0.681.g6f77f65b4e-goog >
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. [...]
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-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. 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. > > [...]
# 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); }; After the necessary DEFINE_* macros, we can then write code like struct MOCK(sender) mock_sender = CONSTRUCT_MOCK(sender, test); /* Fake an error for a specific input. */ handle = KUNIT_EXPECT_CALL(send(<omitted>, kunit_int_eq(42))); handle->action = kunit_int_return(test, -EINVAL); /* Pass the mocked object to some code under test. */ KUNIT_EXPECT_EQ(test, -EINVAL, send_message(...)); I.e. the goal is to make it easier to test 1) with less dependencies (we don't need to setup a real `sender`) 2) unusual/error conditions more easily. In the future, we hope to build upon this to support mocking in more contexts, e.g. standalone funcs, etc. # TODOs ## Naming This introduces a number of new macros for dealing with mocks, e.g: DEFINE_STRUCT_CLASS_MOCK(METHOD(foo), CLASS(example), RETURNS(int), PARAMS(struct example *, int)); ... KUNIT_EXPECT_CALL(foo(mock_get_ctrl(mock_example), ...); For consistency, we could prefix everything with KUNIT, e.g. `KUNIT_DEFINE_STRUCT_CLASS_MOCK` and `kunit_mock_get_ctrl`, but it feels like the names might be long enough that they would hinder readability. ## Usage For now the only use of class mocking is in kunit-example-test.c As part of changing this from an RFC to a real patch set, we're hoping to include at least one example. Pointers to bits of code where this would be useful that aren't too hairy would be appreciated. E.g. could easily add a test for tools/perf/ui/progress.h, e.g. that ui_progress__init() calls ui_progress_ops.init(), but that likely isn't useful to anyone. Brendan Higgins (9): kunit: test: add kunit_stream a std::stream like logger kunit: test: add concept of post conditions checkpatch: add support for struct MOCK(foo) syntax kunit: mock: add parameter list manipulation macros kunit: mock: add internal mock infrastructure kunit: mock: add basic matchers and actions kunit: mock: add class mocking support kunit: mock: add struct param matcher kunit: mock: implement nice, strict and naggy mock distinctions Daniel Latypov (2): Revert "kunit: move string-stream.h to lib/kunit" kunit: expose kunit_set_failure() for use by mocking Marcelo Schmitt (1): kunit: mock: add macro machinery to pick correct format args include/kunit/assert.h | 3 +- include/kunit/kunit-stream.h | 94 +++ include/kunit/mock.h | 902 +++++++++++++++++++++++++ include/kunit/params.h | 305 +++++++++ {lib => include}/kunit/string-stream.h | 2 + include/kunit/test.h | 9 + lib/kunit/Makefile | 9 +- lib/kunit/assert.c | 2 - lib/kunit/common-mocks.c | 409 +++++++++++ lib/kunit/kunit-example-test.c | 90 +++ lib/kunit/kunit-stream.c | 110 +++ lib/kunit/mock-macro-test.c | 241 +++++++ lib/kunit/mock-test.c | 531 +++++++++++++++ lib/kunit/mock.c | 370 ++++++++++ lib/kunit/string-stream-test.c | 3 +- lib/kunit/string-stream.c | 5 +- lib/kunit/test.c | 15 +- scripts/checkpatch.pl | 4 + 18 files changed, 3091 insertions(+), 13 deletions(-) create mode 100644 include/kunit/kunit-stream.h create mode 100644 include/kunit/mock.h create mode 100644 include/kunit/params.h rename {lib => include}/kunit/string-stream.h (95%) create mode 100644 lib/kunit/common-mocks.c create mode 100644 lib/kunit/kunit-stream.c create mode 100644 lib/kunit/mock-macro-test.c create mode 100644 lib/kunit/mock-test.c create mode 100644 lib/kunit/mock.c base-commit: 10b82d5176488acee2820e5a2cf0f2ec5c3488b6