Date:   Tue, 22 Sep 2020 17:24:44 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>,
        David Gow <davidgow@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>
Cc:     KUnit Development <kunit-dev@...glegroups.com>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Alan Maguire <alan.maguire@...cle.com>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [RFC v1 00/12] kunit: introduce class mocking support.

On Fri, Sep 18, 2020 at 11:31 AM Daniel Latypov <dlatypov@...gle.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
>

Powered by blists - more mailing lists