On Mon, Oct 4, 2021 at 10:21 AM Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi David, > > [removing netdev, this is more kunit-related] > > > This looks good to me. I don't think you'll be the only person to hit > > this issue, so -- while it's probably overall nicer if tests can sit > > in their own module -- we'll look into finding a way of supporting > > this with KUnit at some point. > > Just digging in to this a little: what's the intended structure here? Is > it intended to have the tests as their own loadable modules? > > For MCTP, I'd like to enable tests when we're built as a module; in that > case, are you expecting we'd now have two modules: the core, and the > kunit tests? > That's what we've mostly done so far, just because it can be useful to load the core without the tests. There are also a number of test modules for things which themselves are built-in (e.g. KASAN), so having separate test modules was necessary for those. We're still feeling out what the best way to do this is, though, and there's nothing wrong with including the KUnit tests in the MCTP module. It's just not as well explored, so does tend to hit a few issues. Better supporting tests embedded in a larger model (as well as making it easier to export symbols "for testing") are on our roadmap. > As you've seen, my test implementation here is to directly include the > <tests>.c file; this means I don't need to EXPORT_SYMBOL all of the > MCTP-internal functions that I want to test; they can remain private to > the individual compilation unit. However, the current kunit_test_suites > implementation prevents this. We actually already have a test suite which already includes the "tests.c" file: the AppArmor suite. However AppArmor can't be built as a module, so we never hit any of the module issues with it. We're still working out how best to test internal, static functions: the options basically are to include the "test.c" as you've found, or to find some way of exporting symbols "for testing", either with some macro magic (the functions are 'static' if CONFIG_KUNIT is undefined, or similar), or by trying to use module symbol namespaces. I expect we'll have a few tests doing each for a while, and then start to settle on whatever ends up being most convenient in most cases. > I've been hacking around with the (very experimental) patch below, to > allow tests in modules (without open-coding kunit_test_suites, which the > aspeed mmc sdhci driver has done), but I don't have much background on > kunit, so I'm not sure whether this is a useful direction to take. This looks really neat, and much better than the existing module support. There are a few other tests which open-code kunit_test_suites or something similar which have hit problems due to running too early if built-in, which this could help resolve as well. Getting rid of (or at least standardising) those custom KUnit suite initialisers is something I've wanted to do for a while. I haven't had a chance to look through the patch in a lot of detail yet (and there are a few tweaks which would be needed, as it breaks the UML build at the moment), but this definitely looks a much better approach than what we've been doing. Module support has always been a little bit less stable than running built in tests, and this should at least go some way towards reconciling those two paths. In any case, thanks a lot -- this is awesome. Cheers, -- David
