On Fri, Oct 11, 2019 at 6:19 AM Theodore Y. Ts'o tytso@mit.edu wrote:
On Fri, Oct 11, 2019 at 03:05:43AM -0700, Brendan Higgins wrote:
That's an interesting point. Should we try to establish a pattern for how tests should be configured? My *very long term* goal is to eventually have tests able to be built and run without any kind of kernel of any kind, but I don't think that having a single config for all tests in a subsystem gets in the way of that, so I don't think I have a strong preference in terms of what I want to do.
Nevertheless, I think establishing patterns is good. Do we want to try to follow Ted's preference as a general rule from now on?
As I suggested on another thread (started on kunit-dev, but Brendan has cc'ed in linux-kselftest), I think it might really work well if "make kunit" runs all of the kunit tests automatically. As we add more kunit tests, finding all of the CONFIG options so they can be added to the kunitconfig file is going to be hard, so kunit.py really needs an --allconfig which does this automatically.
Along these lines, perhaps we should state that as a general rule the CONFIG option for Kunit tests should only depend on KUINIT, and use select to enable other dependencies. i.e., for the ext4 kunit tests, it should look like this:
config EXT4_KUNIT_TESTS bool "KUnit test for ext4 inode" select EXT4_FS depends on KUNIT ...
Done
In the current patch, we use "depends on EXT4_FS", which meant that when I first added "CONFIG_EXT4_KUNIT_TESTS=y" to the kunitconfig file, I got the following confusing error message:
% ./tools/testing/kunit/kunit.py run Regenerating .config ... ERROR:root:Provided Kconfig is not contained in validated .config!
Using "select EXT4_FS" makes it much easier to enable the ext4 kunit tests in kunitconfig. At the moment requiring that we two lines to kunitconfig to enable ext4 isn't _that_ bad:
CONFIG_EXT4_FS=y CONFIG_EXT4_KUNIT_TESTS=y
but over time, if many subsystems start adding unit tests, the overhead of managing the kunitconfig file is going to get unwieldy. Hence my suggestion that we just make all Kunit CONFIG options depend only on CONFIG_KUNIT.
I agree with Iurii. I don't think that this example alone warrants adding support for being able to read test data in from a separate file (I would also like some clarification here on what is meant by reading in from a separate file). I can imagine some scenarios where that might make sense, but I think it would be better to get more examples before trying to support that use case.
So what I was thinking might happen is that for some of the largest unit tests before I would transition to deciding that xfstests was the better way to go, I *might* have a small, 100k ext4 file system which would checked into the kernel sources as fs/ext4/kunit_test.img, and there would be a makefile rule that would turn that into fs/ext4/kunit_test_img.c file that might look something like:
const ext4_kunit_test_img[] = { 0xde, ...
But I'm not sure I actually want to go down that path. It would certainly better from a test design perspective to create test mocks at a higher layer, such as ext4_iget() and ext4_read_block_bitmap().
The problem is that quite a bit of code in ext4 would have to be *extensively* refactored in order to allow for easy test mocking, since we have calls to sb_bread, ext4_bread(), submit_bh(), etc., sprinkled alongside the code logic that we would want to test.
So using a small test image and making the cut line be at the buffer cache layer is going to be much, *much* simpler at least in the short term. So the big question is how much of an investment (or technical debt paydown) do I want to do right away, versus taking a shortcut to get better unit test coverage more quickly, and then do further tech debt reduction later?
- Ted