Re: [PATCH v3 0/2] kunit: add macro to allow conditionally exposing static symbols to tests

[David Gow]

On Wed, Dec 7, 2022 at 10:43 AM John Johansen
<john.johansen@xxxxxxxxxxxxx> wrote:
>
> On 12/6/22 17:40, Rae Moar wrote:
> > Currently in order to test a static function, tests must be included in the
> > same translation unit as the function. However, this can cause issues with
> > including implementation and test code in the same file. As an alternative,
> > the first patch in this series creates a macro that will set a function to
> > be static or not depending on whether CONFIG_KUNIT is enabled. This allows
> > the function to be visible during testing and static otherwise.
> >
> > As an example, the current status quo to test static functions is:
> >
> > === test.c ===
> >
> > static void test_case(struct kunit *test)
> > {
> >    KUNIT_EXPECT_EQ(test, my_func_to_test(), 2);
> > }
> >
> > Then the tests are included in the implementation file as a workaround to
> > the issue of testing static functions:
> >
> > === implementation.c ===
> >
> > static int my_func_to_test() {...}
> > ...
> > #include "test.c"
> >
> > Instead, the function could be defined with this new macro:
> >
> > === implementation.c ===
> >
> > VISIBLE_IF_KUNIT int my_func_to_test() {...}
> >
> > The first patch also creates a macro that will export a symbol into a kunit
> > testing namespace only if CONFIG_KUNIT is enabled. This follows the logic
> > above and allows symbols to be conditionally exported based on the testing
> > status.
> >
> > The second patch in the series updates the policy_unpack test in AppArmor
> > to show an example of how to use both of these macros in order to address
> > the issue of testing static functions. Additionally, the patch allows the
> > policy_unpack test to be built as a module.
> >
> > Changes since v2:
> >   - Add mention of namespacing symbols to the commit message of the
> >     second patch.
> >   - Change module name in the second patch from policy_unpack_test to
> >     apparmor_policy_unpack_test.
> >
> > Changes since v1:
> >   - Changed the namespace of exported symbols for the apparmor
> >     policy_unpack_test by adding the aa_ prefix.
> >   - Separated the documentation comments for macros in
> >     include/kunit/visibility.h.
> >   - Changed copyright date and author for include/kunit/visibility.h.
> >
> > Rae Moar (2):
> >    kunit: add macro to allow conditionally exposing static symbols to
> >      tests
> >    apparmor: test: make static symbols visible during kunit testing
> >
> >   include/kunit/visibility.h                |  33 +++
> >   security/apparmor/Kconfig                 |   4 +-
> >   security/apparmor/Makefile                |   3 +
> >   security/apparmor/include/policy_unpack.h |  50 +++++
> >   security/apparmor/policy_unpack.c         | 238 ++++++++++------------
> >   security/apparmor/policy_unpack_test.c    |  69 ++++---
> >   6 files changed, 229 insertions(+), 168 deletions(-)
> >   create mode 100644 include/kunit/visibility.h
> >
> >
> > base-commit: 0f08f3e2a0186dfb8e33cb46105228eb18448a0e
>
> thanks Rae,
>
> looks good to me, David unless you tell me otherwise I assume this is
> still going in via the kselftest/kunit tree.
>

Yup, both of these are looking good to me (and work fine on my machine).

I've added these to the queue for the kselftest/kunit tree. There's a
chance they won't make it into 6.2, but if not, they should be the
first things in the 6.3 branch.

Thanks very much,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature