| Message ID | 20210922182541.1372400-1-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kfence: test: use kunit_skip() to skip tests | expand |
On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote: > > Use the new kunit_skip() to skip tests if requirements were not met. It > makes it easier to see in KUnit's summary if there were skipped tests. > > Signed-off-by: Marco Elver <elver@google.com> > --- Thanks: I'm glad these features are proving useful. I've tested these under qemu, and it works pretty well. Certainly from the KUnit point of view, this is: Reviewed-by: David Gow <davidgow@google.com> (A couple of unrelated complaints about the kfence tests are that TRACEPOINTS isn't selected by default, and that the manual registering/unregistering of the tracepoints does break some of the kunit tooling when several tests are built-in. That's something that exists independently of this patch, though, and possibly requires some KUnit changes to be fixed cleanly (kfence isn't the only thing to do this). So not something to hold up this patch.) Cheers, -- David > mm/kfence/kfence_test.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index f1690cf54199..695030c1fff8 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -32,6 +32,11 @@ > #define arch_kfence_test_address(addr) (addr) > #endif > > +#define KFENCE_TEST_REQUIRES(test, cond) do { \ > + if (!(cond)) \ > + kunit_skip((test), "Test requires: " #cond); \ > +} while (0) > + > /* Report as observed from console. */ > static struct { > spinlock_t lock; > @@ -555,8 +560,7 @@ static void test_init_on_free(struct kunit *test) > }; > int i; > > - if (!IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) > - return; > + KFENCE_TEST_REQUIRES(test, IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)); > /* Assume it hasn't been disabled on command line. */ > > setup_test_cache(test, size, 0, NULL); > @@ -603,10 +607,8 @@ static void test_gfpzero(struct kunit *test) > char *buf1, *buf2; > int i; > > - if (CONFIG_KFENCE_SAMPLE_INTERVAL > 100) { > - kunit_warn(test, "skipping ... would take too long\n"); > - return; > - } > + /* Skip if we think it'd take too long. */ > + KFENCE_TEST_REQUIRES(test, CONFIG_KFENCE_SAMPLE_INTERVAL <= 100); > > setup_test_cache(test, size, 0, NULL); > buf1 = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY); > -- > 2.33.0.464.g1972c5931b-goog >
On Thu, 23 Sept 2021 at 19:39, David Gow <davidgow@google.com> wrote: > On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote: > > > > Use the new kunit_skip() to skip tests if requirements were not met. It > > makes it easier to see in KUnit's summary if there were skipped tests. > > > > Signed-off-by: Marco Elver <elver@google.com> > > --- > > Thanks: I'm glad these features are proving useful. I've tested these > under qemu, and it works pretty well. > > Certainly from the KUnit point of view, this is: > Reviewed-by: David Gow <davidgow@google.com> Thanks! > (A couple of unrelated complaints about the kfence tests are that > TRACEPOINTS isn't selected by default, and that the manual > registering/unregistering of the tracepoints does break some of the > kunit tooling when several tests are built-in. That's something that > exists independently of this patch, though, and possibly requires some > KUnit changes to be fixed cleanly (kfence isn't the only thing to do > this). So not something to hold up this patch.) I think there was a reason we wanted it to "depends on TRACEPOINTS". If it were to select it, then if you do a CONFIG_KUNIT_ALL_TESTS=y, and also have KFENCE on, you'll always select tracepoints. In certain situations this may not be wanted. If we didn't have CONFIG_KUNIT_ALL_TESTS, then certainly, auto-selecting TRACEPOINTS would be ok. If you can live with that, we can of course switch it to do "select TRACEPOINTS". On a whole I err on the side of fewer auto-selected Kconfig options. Thanks, -- Marco
On Fri, Sep 24, 2021 at 1:58 AM Marco Elver <elver@google.com> wrote: > > On Thu, 23 Sept 2021 at 19:39, David Gow <davidgow@google.com> wrote: > > On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote: > > > > > > Use the new kunit_skip() to skip tests if requirements were not met. It > > > makes it easier to see in KUnit's summary if there were skipped tests. > > > > > > Signed-off-by: Marco Elver <elver@google.com> > > > --- > > > > Thanks: I'm glad these features are proving useful. I've tested these > > under qemu, and it works pretty well. > > > > Certainly from the KUnit point of view, this is: > > Reviewed-by: David Gow <davidgow@google.com> > > Thanks! > > > (A couple of unrelated complaints about the kfence tests are that > > TRACEPOINTS isn't selected by default, and that the manual > > registering/unregistering of the tracepoints does break some of the > > kunit tooling when several tests are built-in. That's something that > > exists independently of this patch, though, and possibly requires some > > KUnit changes to be fixed cleanly (kfence isn't the only thing to do > > this). So not something to hold up this patch.) > > I think there was a reason we wanted it to "depends on TRACEPOINTS". > If it were to select it, then if you do a CONFIG_KUNIT_ALL_TESTS=y, > and also have KFENCE on, you'll always select tracepoints. In certain > situations this may not be wanted. If we didn't have > CONFIG_KUNIT_ALL_TESTS, then certainly, auto-selecting TRACEPOINTS > would be ok. > > If you can live with that, we can of course switch it to do "select > TRACEPOINTS". That's probably more convenient for me, but I confess that my use case is almost always wanting to run the KUnit tests, so I'm not unbiased. :-) > > On a whole I err on the side of fewer auto-selected Kconfig options. Yeah, it's perfectly sensible to do it either way. Maybe the right option is to have a .kunitconfig file which has TRACEPOINTS enabled. It's probably not worth doing if there's still issues with kunit_tool parsing the results when the test is built-in, so this should probably wait until KUnit has a way of running code on init/exit of suites as well as individual tests within those suites. KFENCE is not the only test suite which needs something like that (nor the only one which does some module_init or late_initcall stuff which causes some formatting issues with builtin tests). Cheers, -- David
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index f1690cf54199..695030c1fff8 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -32,6 +32,11 @@ #define arch_kfence_test_address(addr) (addr) #endif +#define KFENCE_TEST_REQUIRES(test, cond) do { \ + if (!(cond)) \ + kunit_skip((test), "Test requires: " #cond); \ +} while (0) + /* Report as observed from console. */ static struct { spinlock_t lock; @@ -555,8 +560,7 @@ static void test_init_on_free(struct kunit *test) }; int i; - if (!IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) - return; + KFENCE_TEST_REQUIRES(test, IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)); /* Assume it hasn't been disabled on command line. */ setup_test_cache(test, size, 0, NULL); @@ -603,10 +607,8 @@ static void test_gfpzero(struct kunit *test) char *buf1, *buf2; int i; - if (CONFIG_KFENCE_SAMPLE_INTERVAL > 100) { - kunit_warn(test, "skipping ... would take too long\n"); - return; - } + /* Skip if we think it'd take too long. */ + KFENCE_TEST_REQUIRES(test, CONFIG_KFENCE_SAMPLE_INTERVAL <= 100); setup_test_cache(test, size, 0, NULL); buf1 = test_alloc(test, size, GFP_KERNEL, ALLOCATE_ANY);
Use the new kunit_skip() to skip tests if requirements were not met. It makes it easier to see in KUnit's summary if there were skipped tests. Signed-off-by: Marco Elver <elver@google.com> --- mm/kfence/kfence_test.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)