[PATCH] kunit: mock: add support for function mocks with no parameters

[Marcelo Schmitt]

Function mocks defined with DEFINE_FUNCTION_MOCK(...) do not support
empty parameters list due to strict function prototypes enforcement
(-Werror=strict-prototypes). Add support for function mocks with no
parameters by adding checks to declare strict function prototypes when
an empty param list is provided.
Further, add an expectation to test that the generated code works.

Co-developed-by: Anderson Reis Rosa <anderson...@gmail.com>
Signed-off-by: Anderson Reis Rosa <anderson...@gmail.com>
Signed-off-by: Marcelo Schmitt <marcelo....@gmail.com>
---
We noted that the proposed changes added a few checkpatch errors.
However, as checkpatch reported the same errors for other macros at
params.h, we didn't spend a lot of time trying to find a fix for them.

We hope this may solve Bugzilla Bug 205495
https://bugzilla.kernel.org/show_bug.cgi?id=205495

include/test/mock.h | 2 +-
include/test/params.h | 12 +++++++++++-
test/mock-macro-test.c | 6 ++++++
3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/test/mock.h b/include/test/mock.h
index 8b8031c13b2a..c46c90abc12a 100644
--- a/include/test/mock.h
+++ b/include/test/mock.h
@@ -640,7 +640,7 @@ int mock_in_sequence(struct KUNIT_T *test, struct mock_expectation *first, ...);
return_type, \
RETURN, \
param_types...) \
- return_type name(PARAM_LIST_FROM_TYPES(param_types)) \
+ return_type name(FUNC_PARAM_LIST_FROM_TYPES(param_types)) \
{ \
struct mock *mock = MOCK_SOURCE(mock_source_ctx, \
handle_index); \
diff --git a/include/test/params.h b/include/test/params.h
index 50d54035175d..ca4689dd0576 100644
--- a/include/test/params.h
+++ b/include/test/params.h
@@ -264,6 +264,11 @@
not_used, \
args)

+#define FUNC_PARAM_LIST_FROM_TYPES(args...) \
+ IF(IS_EQUAL(NUM_VA_ARGS(args), 0))(void) \
+ IF(IS_NOT_EQUAL(NUM_VA_ARGS(args), 0)) \
+ (PARAM_LIST_FROM_TYPES(args))
+
#define PRODUCE_TYPE_NAME(context, type, index) #type
#define TYPE_NAMES_FROM_TYPES(handle_index, args...) \
FOR_EACH_PARAM(PRODUCE_TYPE_NAME, \
@@ -282,12 +287,17 @@
IF(IS_EQUAL(index, ctrl_index))(struct mock *arg##ctrl_index) \
IF(IS_NOT_EQUAL(index, ctrl_index))( \
struct mock_param_matcher *arg##index)
-#define MATCHER_PARAM_LIST_FROM_TYPES(ctrl_index, args...) \
+#define MATCHER_PARAM_LIST_FROM_TYPES_INTERNAL(ctrl_index, args...) \
FOR_EACH_PARAM(PRODUCE_MATCHER_AND_ARG, \
FILTER_NONE, \
ctrl_index, \
args)

+#define MATCHER_PARAM_LIST_FROM_TYPES(ctrl_index, args...) \
+ IF(IS_EQUAL(NUM_VA_ARGS(args), 0))(void) \
+ IF(IS_NOT_EQUAL(NUM_VA_ARGS(args), 0)) \
+ (MATCHER_PARAM_LIST_FROM_TYPES_INTERNAL(ctrl_index, args))
+
#define PRODUCE_ARG(context, type, index) arg##index
#define ARG_NAMES_FROM_TYPES(ctrl_index, args...) \
FOR_EACH_PARAM(PRODUCE_ARG, \
diff --git a/test/mock-macro-test.c b/test/mock-macro-test.c
index 14da7ebe752d..91a926558551 100644
--- a/test/mock-macro-test.c
+++ b/test/mock-macro-test.c
@@ -59,6 +59,8 @@ DEFINE_VOID_CLASS_MOCK_HANDLE_INDEX(METHOD(test_void_ptr_func),

DEFINE_FUNCTION_MOCK(add, RETURNS(int), PARAMS(int, int));

+DEFINE_FUNCTION_MOCK(no_param, RETURNS(int), PARAMS());
+
struct mock_macro_context {
struct MOCK(test_struct) *mock_test_struct;
struct MOCK(void) *mock_void_ptr;
@@ -217,7 +219,11 @@ static void mock_macro_test_generated_function_code_works(struct KUNIT_T *test)
handle = EXPECT_CALL(add(int_eq(test, 4), int_eq(test, 3)));
handle->action = int_return(test, 7);

+ handle = EXPECT_CALL(no_param());
+ handle->action = int_return(test, 0);
+
EXPECT_EQ(test, 7, add(4, 3));
+ EXPECT_EQ(test, 0, no_param());
}

static int mock_macro_test_init(struct KUNIT_T *test)
--
2.33.0

[Daniel Latypov]

On Sat, Sep 18, 2021 at 12:44 PM Marcelo Schmitt
<marcelo....@gmail.com> wrote:
>
> Function mocks defined with DEFINE_FUNCTION_MOCK(...) do not support
> empty parameters list due to strict function prototypes enforcement
> (-Werror=strict-prototypes). Add support for function mocks with no
> parameters by adding checks to declare strict function prototypes when
> an empty param list is provided.
> Further, add an expectation to test that the generated code works.
>
> Co-developed-by: Anderson Reis Rosa <anderson...@gmail.com>
> Signed-off-by: Anderson Reis Rosa <anderson...@gmail.com>
> Signed-off-by: Marcelo Schmitt <marcelo....@gmail.com>

Meta: kunit/alpha/master isn't really maintained anymore.
I think David and myself having some commits from this year might give
the wrong impression.
But all of my patches in 2021 were to make it easier to get people to
*move away* from kunit/alpha/master ;)

We sort of don't want to encourage anyone new from using it, see
https://kunit.dev/third_party/stable_kernel/index.html
That said, it's useful to have it around as a reference, and this
patch makes it more useful, so:

Reviewed-by: Daniel Latypov <dlat...@google.com>
This looks reasonable to me, minor nit about the added test case below.

I think only Brendan could manually apply this and push it.
It would be a bit easier to go through the process of pushing it to
gerrit at https://kunit-review.googlesource.com/
I'd wait on Brendan to chime in on what he'd like to do (or if he'd
rather not have any new changes).

I think this test case would be a bit more readable if restructured as
219 handle = EXPECT_CALL(add(int_eq(test, 4), int_eq(test, 3)));
220 handle->action = int_return(test, 7);
221 EXPECT_EQ(test, 7, add(4, 3));
222
223 handle = EXPECT_CALL(no_param());
224 handle->action = int_return(test, 0);
225 EXPECT_EQ(test, 0, no_param());

I might also further suggest using a different return value than 0.
Some frameworks return 0-values by default when there's no specified
action, in which case this EXPECT_EQ() would pass even if we didn't
specify `int_return(test, 0)`.
That's not how KUnit works, so it's not actually relevant.

>
> static int mock_macro_test_init(struct KUNIT_T *test)
> --
> 2.33.0
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/YUZBkZhQsF5SlcLb%40marsc.168.1.7.

[Marcelo Schmitt]

Hi Daniel,

Thanks for your review.

On 09/21, Daniel Latypov wrote:
> On Sat, Sep 18, 2021 at 12:44 PM Marcelo Schmitt
> <marcelo....@gmail.com> wrote:
> >
> > Function mocks defined with DEFINE_FUNCTION_MOCK(...) do not support
> > empty parameters list due to strict function prototypes enforcement
> > (-Werror=strict-prototypes). Add support for function mocks with no
> > parameters by adding checks to declare strict function prototypes when
> > an empty param list is provided.
> > Further, add an expectation to test that the generated code works.
> >
> > Co-developed-by: Anderson Reis Rosa <anderson...@gmail.com>
> > Signed-off-by: Anderson Reis Rosa <anderson...@gmail.com>
> > Signed-off-by: Marcelo Schmitt <marcelo....@gmail.com>
>
> Meta: kunit/alpha/master isn't really maintained anymore.
> I think David and myself having some commits from this year might give
> the wrong impression.
> But all of my patches in 2021 were to make it easier to get people to
> *move away* from kunit/alpha/master ;)

We can't submit it upstream because the mock stuff isn't there yet.
By the way, as nothing from mocking is upstream and kunit/alpha/master is
being frozen somewhat, what tree/branch should we base our work on if we
decide to develop more on the mocking framework?
I recall the branch with the POC for mocking was at
https://kunit-review.googlesource.com/c/linux/+/1114
Should we use this branch to base future work on mocking?
Or will the mocking framework be discontinued?

Sorry for asking so many questions. We just want to help to enhance KUnit.
We can work on something else besides mocking if it makes more sense to the
project.

>
> We sort of don't want to encourage anyone new from using it, see
> https://kunit.dev/third_party/stable_kernel/index.html
> That said, it's useful to have it around as a reference, and this
> patch makes it more useful, so:
>
> Reviewed-by: Daniel Latypov <dlat...@google.com>
> This looks reasonable to me, minor nit about the added test case below.

Thanks. We'll send a v2 in case Brendan decides to pick these changes up.

[Marcelo Schmitt]

Function mocks defined with DEFINE_FUNCTION_MOCK(...) do not support
empty parameters list due to strict function prototypes enforcement
(-Werror=strict-prototypes). Add support for function mocks with no
parameters by adding checks to declare strict function prototypes when
an empty param list is provided.
Further, add an expectation to test that the generated code works.

Co-developed-by: Anderson Reis Rosa <anderson...@gmail.com>
Signed-off-by: Anderson Reis Rosa <anderson...@gmail.com>
Signed-off-by: Marcelo Schmitt <marcelo....@gmail.com>

Reviewed-by: Daniel Latypov <dlat...@google.com>
---
Not sure if we are allowed to add a Reported-by tag to credit Andy Li, so we didn't.

include/test/mock.h | 2 +-
include/test/params.h | 12 +++++++++++-

test/mock-macro-test.c | 7 ++++++-
3 files changed, 18 insertions(+), 3 deletions(-)

index 14da7ebe752d..d97cf3642bb3 100644

--- a/test/mock-macro-test.c
+++ b/test/mock-macro-test.c
@@ -59,6 +59,8 @@ DEFINE_VOID_CLASS_MOCK_HANDLE_INDEX(METHOD(test_void_ptr_func),

DEFINE_FUNCTION_MOCK(add, RETURNS(int), PARAMS(int, int));

+DEFINE_FUNCTION_MOCK(no_param, RETURNS(int), PARAMS());
+
struct mock_macro_context {
struct MOCK(test_struct) *mock_test_struct;
struct MOCK(void) *mock_void_ptr;

@@ -216,8 +218,11 @@ static void mock_macro_test_generated_function_code_works(struct KUNIT_T *test)

handle = EXPECT_CALL(add(int_eq(test, 4), int_eq(test, 3)));
handle->action = int_return(test, 7);

-

EXPECT_EQ(test, 7, add(4, 3));
+

+ handle = EXPECT_CALL(no_param());
+ handle->action = int_return(test, 9);
+ EXPECT_EQ(test, 9, no_param());

[Daniel Latypov]

[2]
On Sat, Sep 25, 2021 at 12:30 PM Marcelo Schmitt

All the mocking stuff is in limbo at the moment.
The v2 of the class mocking RFC was sent out Oct 2020,
https://lore.kernel.org/linux-kselftest/20201012222050.9...@google.com/

Until we have user interest in mocking support, that RFC will likely
just sit there.
Or maybe we scrap it and just introduce the functionality piece by piece.

There's https://kunit.dev/mocking.html which talks about how one can
implement mocking on their own, or (better yet) write fakes while
leveraging KUnit.
If we start seeing adoption of that, we can start factoring out
functionality into shared code as needed, e.g. support for saying a
mock should be called N times, then maybe gradually the parameter
matchers and return values, etc.

I missed it, but I know David and Brendan talked a bit about this in
their recent LPC talk, https://youtu.be/Y_minEhZNm8?t=15905

>
> Sorry for asking so many questions. We just want to help to enhance KUnit.

No worries, and we really appreciate it.

> We can work on something else besides mocking if it makes more sense to the
> project.

Mocking doesn't feel like an area where we can expect to see progress right now.
In terms of other KUnit features we know would be useful now, I think
it's mostly just [1] and [2], which hopefully will land in 5.16.

I think right now we probably need more tests written to have a better
idea of what else we could/should do.
Partly because of that, David is trying to get the ball rolling on
testing ext4. We're also hopeful that it'll be easier to add tests if
adjacent code is already tested (sharing fakes, conventions, ability
to copy-paste, etc.).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33
[2] https://lore.kernel.org/linux-kselftest/20210909001037.2...@google.com/

[Daniel Latypov]

To be clear, if anyone thinks up a useful feature, that'd be great.
I personally am just out of ideas at the moment, and I think so are
Brendan and David.

We'd want to prioritize features that can improve existing tests or
unblock known new tests.
Mocking in the alpha version of KUnit is a case where a feature
sounded really good on paper and had a bunch of bells and whistles
(e.g. strict/nice/naggy mocks support, etc.) but was perhaps
overengineered and thus failed to find a home upstream.

But I just thought of a few more things we could do in the kunit.py script.
I think we have more room for improvement there than in the in-kernel
part of KUnit right now, but I assume it's the more boring part for
most people.

One thing I'd really like to see is getting code coverage to work in
kunit.py while using QEMU.
We have a process for doing so under UML here:
https://lore.kernel.org/linux-kselftest/20210901190623...@google.com/
UML actually uses a different coverage implementation than normal, so
there's a few things that would need to change.

We can build and run against "normal" coverage kernels pretty easily:

$ cat >qemu_coverage_kunitconfig <<EOF
CONFIG_KUNIT=y
CONFIG_KUNIT_EXAMPLE_TEST=y
CONFIG_GCOV_KERNEL=y
CONFIG_DEBUG_FS=y
CONFIG_GCOV_PROFILE_ALL=y
EOF
$ ./tools/testing/kunit/kunit.py run --arch=x86_64
--kunitconfig=qemu_coverage_kunitconfig

The problem is we'd need to copy the coverage data off the VM instead
of just letting it shutdown when tests are done.
If we had a userspace running, we'd basically do something like
$ scp -r user@vm:/sys/kernel/debug/gcov .
<some stuff to get these files in the right spot under .kunit/>
<then we'd run lcov and genhtml, just like we do for UML>

Normal KUnit tests definitely don't want to have to have the overhead
of running a userspace, so the implementation might look like a
"--qemu_coverage" flag, or maybe a set of generic flags that would
give a user enough control over the VM to do this.
Or maybe the right answer is to not involve kunit.py at all.

Not sure if that sounds interesting to you or anyone.

[Daniel Latypov]

wrong copy-paste, meant
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#generating-code-coverage-reports-under-uml

[Marcelo Schmitt]

Removed Andy from the CC list as we're not talking about that bug anymore.
[...]

Sounds cool. We'll try to make it.

Thanks