Re: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test
On Wed, Feb 21, 2024 at 4:28 AM David Gow wrote:
>
> KUnit's executor_test logs the filter string in KUNIT_ASSERT_EQ_MSG(),
> but passed a random character from the filter, rather than the whole
> string.
>
> This was found by annotating KUNIT_ASSERT_EQ_MSG() to let gcc validate
> the format string.
>
> Fixes: 76066f93f1df ("kunit: add tests for filtering attributes")
> Signed-off-by: David Gow
Hello!
This change looks good to me. Thanks for fixing this mistake.
Thanks!
-Rae
Reviewed-by: Rae Moar
> ---
> lib/kunit/executor_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index 22d4ee86dbed..3f7f967e3688 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -129,7 +129,7 @@ static void parse_filter_attr_test(struct kunit *test)
> GFP_KERNEL);
> for (j = 0; j < filter_count; j++) {
> parsed_filters[j] = kunit_next_attr_filter(&filter, &err);
> - KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter
> '%s'", filters[j]);
> + KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter
> from '%s'", filters);
> }
>
> KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[0]),
> "speed");
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
Re: [PATCH 1/2] kunit: Warn if tests are slow
On Mon, Sep 11, 2023 at 5:51 AM Maxime Ripard wrote:
>
> Kunit recently gained support to setup attributes, the first one being
> the speed of a given test, then allowing to filter out slow tests.
>
> A slow test is defined in the documentation as taking more than one
> second. There's an another speed attribute called "super slow" but whose
> definition is less clear.
>
> Add support to the test runner to check the test execution time, and
> report tests that should be marked as slow but aren't.
>
> Signed-off-by: Maxime Ripard
Hi!
I like this idea especially if it was helpful in identifying slow
tests already! I have a few thoughts on this. I share Jani's concern
for warning all tests on slow machines. I can think of a few options.
First, we could increase the threshold to about 2s even though that
would eliminate warnings on potentially slow tests. However, this
would point out the slowest tests.
Second, we could change this to warn users only when they choose by
making this a configurable option or making this a script to output a
list of all unmarked slow tests.
Third, we could leave this as is. As the KUnit warnings do not show up
in the kunit.py output and do not cause the test to fail in any way
they are relatively harmless if they are unwanted by the user.
Not quite sure which I prefer? The second option might be the cleanest
for the user and the time threshold could even be customizable. Let me
know what you think.
> ---
> lib/kunit/test.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..a3b924501f3d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -379,6 +379,9 @@ static void kunit_run_case_internal(struct kunit *test,
> struct kunit_suite *suite,
> struct kunit_case *test_case)
> {
> + struct timespec64 start, end;
> + struct timespec64 duration;
> +
> if (suite->init) {
> int ret;
>
> @@ -390,7 +393,20 @@ static void kunit_run_case_internal(struct kunit *test,
> }
> }
>
> + ktime_get_ts64(&start);
> +
> test_case->run_case(test);
> +
> + ktime_get_ts64(&end);
> +
> + duration = timespec64_sub(end, start);
> +
> + if (duration.tv_sec >= 1 &&
> + (test_case->attr.speed == KUNIT_SPEED_UNSET ||
> +test_case->attr.speed >= KUNIT_SPEED_NORMAL))
> + kunit_warn(test,
> + "Test should be marked slow (runtime:
> %lld.%09lds)",
> + duration.tv_sec, duration.tv_nsec);
I would consider moving this if statement into a separate function.
> }
>
> static void kunit_case_internal_cleanup(struct kunit *test)
>
> --
> 2.41.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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kunit-dev/20230911-kms-slow-tests-v1-1-d3800a69a1a1%40kernel.org.
Re: [PATCH RFC 2/2] drm: add documentation for drm_buddy_test kUnit test
On Fri, Sep 1, 2023 at 3:11 AM Mauro Carvalho Chehab wrote: > > Hi Rae, > > Em Thu, 13 Jul 2023 17:31:19 -0400 > Rae Moar escreveu: > > > On Wed, Jul 12, 2023 at 10:29 AM Mauro Carvalho Chehab > > wrote: > > > > > As an example for the new documentation tool, add a documentation > > > for drm_buddy_test. > > > > > > I opted to place this on a completely different directory, in order > > > to make easier to test the feature with: > > > > > > $ make SPHINXDIRS="tests" htmldocs > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > --- > > > > > > To avoid mailbombing on a large number of people, only mailing lists were > > > C/C on the cover. > > > See [PATCH RFC 0/2] at: > > > https://lore.kernel.org/all/cover.1689171160.git.mche...@kernel.org/ > > > > > > Documentation/index.rst| 2 +- > > > Documentation/tests/index.rst | 6 ++ > > > Documentation/tests/kunit.rst | 5 + > > > drivers/gpu/drm/tests/drm_buddy_test.c | 12 > > > 4 files changed, 24 insertions(+), 1 deletion(-) > > > create mode 100644 Documentation/tests/index.rst > > > create mode 100644 Documentation/tests/kunit.rst > > > > > > diff --git a/Documentation/index.rst b/Documentation/index.rst > > > index 9dfdc826618c..80a6ce14a61a 100644 > > > --- a/Documentation/index.rst > > > +++ b/Documentation/index.rst > > > @@ -60,7 +60,7 @@ Various other manuals with useful information for all > > > kernel developers. > > > fault-injection/index > > > livepatch/index > > > rust/index > > > - > > > + test/index > > > > > > User-oriented documentation > > > === > > > diff --git a/Documentation/tests/index.rst b/Documentation/tests/index.rst > > > new file mode 100644 > > > index ..bfc39eb5c0aa > > > --- /dev/null > > > +++ b/Documentation/tests/index.rst > > > @@ -0,0 +1,6 @@ > > > + > > > +Kunit documentation test > > > + > > > + > > > +.. toctree:: > > > + kunit > > > diff --git a/Documentation/tests/kunit.rst b/Documentation/tests/kunit.rst > > > new file mode 100644 > > > index ..6ffc151988a0 > > > --- /dev/null > > > +++ b/Documentation/tests/kunit.rst > > > @@ -0,0 +1,5 @@ > > > +Kunit tests > > > +--- > > > + > > > +.. include-test:: drivers/gpu/drm/tests/drm_buddy_test.c > > > + > > > diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c > > > b/drivers/gpu/drm/tests/drm_buddy_test.c > > > index 09ee6f6af896..dd6c5afd6cd6 100644 > > > --- a/drivers/gpu/drm/tests/drm_buddy_test.c > > > +++ b/drivers/gpu/drm/tests/drm_buddy_test.c > > > @@ -737,6 +737,18 @@ static int drm_buddy_suite_init(struct kunit_suite > > > *suite) > > > return 0; > > > } > > > > > > +/** > > > + * KTEST_SUITE: set of tests for drm buddy alloc > > > + * Scope: drm subsystem > > > + * Mega feature: drm > > > + * Feature: buddy_alloc > > > + * > > > + * KTEST_TEST: drm_test_buddy_alloc_%s > > > + * Description: Run DRM buddy allocation %arg[1] test > > > + * > > > + * arg[1].values: limit, range, optimistic, smoke, pathological > > > + */ > > > > > > Hi! > > > > This is such a cool patch series. I just have a few comments related to the > > output. > > Thank you for your comments! Sorry for not answering earlier. I took some > vacations and this series ended sleeping over other tasks on my > todo list. > > Also, before sending another version, I wanted to have the test_list.py > changes to make it generic enough to be merged on IGT, to avoid having > a fork of it. Those got merged today. Hi Mauro! No worries at all! > > > In the html output the tests are listed as: > > ktest@drm_buddy_test@… > > > > I wonder if instead of using the file name of “drm_buddy_test” this could > > possibly be the suite name, “drm_buddy”, as this is what users will call > > when using kunit.py to run the tests. Although "drm_buddy_test" is also the > > module name so I don't mind it too much. But in the future the file name > > and module name are not guaranteed to be the same for other tests. > >
Re: [PATCH RFC 2/2] drm: add documentation for drm_buddy_test kUnit test
On Wed, Jul 12, 2023 at 10:29 AM Mauro Carvalho Chehab
wrote:
>
> As an example for the new documentation tool, add a documentation
> for drm_buddy_test.
>
> I opted to place this on a completely different directory, in order
> to make easier to test the feature with:
>
> $ make SPHINXDIRS="tests" htmldocs
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were C/C
> on the cover.
> See [PATCH RFC 0/2] at:
> https://lore.kernel.org/all/cover.1689171160.git.mche...@kernel.org/
>
> Documentation/index.rst| 2 +-
> Documentation/tests/index.rst | 6 ++
> Documentation/tests/kunit.rst | 5 +
> drivers/gpu/drm/tests/drm_buddy_test.c | 12
> 4 files changed, 24 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/tests/index.rst
> create mode 100644 Documentation/tests/kunit.rst
>
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index 9dfdc826618c..80a6ce14a61a 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -60,7 +60,7 @@ Various other manuals with useful information for all
> kernel developers.
> fault-injection/index
> livepatch/index
> rust/index
> -
> + test/index
>
> User-oriented documentation
> ===
> diff --git a/Documentation/tests/index.rst b/Documentation/tests/index.rst
> new file mode 100644
> index ..bfc39eb5c0aa
> --- /dev/null
> +++ b/Documentation/tests/index.rst
> @@ -0,0 +1,6 @@
> +
> +Kunit documentation test
> +
> +
> +.. toctree::
> + kunit
> diff --git a/Documentation/tests/kunit.rst b/Documentation/tests/kunit.rst
> new file mode 100644
> index ..6ffc151988a0
> --- /dev/null
> +++ b/Documentation/tests/kunit.rst
> @@ -0,0 +1,5 @@
> +Kunit tests
> +---
> +
> +.. include-test:: drivers/gpu/drm/tests/drm_buddy_test.c
> +
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c
> b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 09ee6f6af896..dd6c5afd6cd6 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -737,6 +737,18 @@ static int drm_buddy_suite_init(struct kunit_suite
> *suite)
> return 0;
> }
>
> +/**
> + * KTEST_SUITE: set of tests for drm buddy alloc
> + * Scope: drm subsystem
> + * Mega feature: drm
> + * Feature: buddy_alloc
> + *
> + * KTEST_TEST: drm_test_buddy_alloc_%s
> + * Description: Run DRM buddy allocation %arg[1] test
> + *
> + * arg[1].values: limit, range, optimistic, smoke, pathological
> + */
> +
I apologize that the last email included a HTML attachment of the message.
Just in case anyone was unable to receive the last email here is a
copy of the message:
Hi!
This is such a cool patch series. I just have a few comments related
to the output.
In the html output the tests are listed as:
ktest@drm_buddy_test@...
I wonder if instead of using the file name of "drm_buddy_test" this
could possibly be the suite name, "drm_buddy", as this is what users
will call when using kunit.py to run the tests. Although
"drm_buddy_test" is also the module name so I don't mind it too much.
But in the future the file name and module name are not guaranteed to
be the same for other tests.
Most preferably, there would be a reference to the kunit suite name,
file name, and the module name.
This may be difficult to implement as these can all differ. I am
currently working on the KUnit Attribute framework which saves the
module name and I am thinking about also saving the file path as a
future attribute. This could be a helpful framework for the KUnit
tests specifically.
I am not sure how easy it would be to access c objects/functions using
this system.
Finally, I was wondering if it is the intention to put a list of all
KUnit tests that use this new feature into tests/kunit.rst or would
this be broken up in some way.
Thanks!
-Rae
> static struct kunit_case drm_buddy_tests[] = {
> KUNIT_CASE(drm_test_buddy_alloc_limit),
> KUNIT_CASE(drm_test_buddy_alloc_range),
> --
> 2.40.1
>
Re: [PATCH RFC 2/2] drm: add documentation for drm_buddy_test kUnit test
On Wed, Jul 12, 2023 at 10:29 AM Mauro Carvalho Chehab
wrote:
> As an example for the new documentation tool, add a documentation
> for drm_buddy_test.
>
> I opted to place this on a completely different directory, in order
> to make easier to test the feature with:
>
> $ make SPHINXDIRS="tests" htmldocs
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were
> C/C on the cover.
> See [PATCH RFC 0/2] at:
> https://lore.kernel.org/all/cover.1689171160.git.mche...@kernel.org/
>
> Documentation/index.rst| 2 +-
> Documentation/tests/index.rst | 6 ++
> Documentation/tests/kunit.rst | 5 +
> drivers/gpu/drm/tests/drm_buddy_test.c | 12
> 4 files changed, 24 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/tests/index.rst
> create mode 100644 Documentation/tests/kunit.rst
>
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index 9dfdc826618c..80a6ce14a61a 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -60,7 +60,7 @@ Various other manuals with useful information for all
> kernel developers.
> fault-injection/index
> livepatch/index
> rust/index
> -
> + test/index
>
> User-oriented documentation
> ===
> diff --git a/Documentation/tests/index.rst b/Documentation/tests/index.rst
> new file mode 100644
> index ..bfc39eb5c0aa
> --- /dev/null
> +++ b/Documentation/tests/index.rst
> @@ -0,0 +1,6 @@
> +
> +Kunit documentation test
> +
> +
> +.. toctree::
> + kunit
> diff --git a/Documentation/tests/kunit.rst b/Documentation/tests/kunit.rst
> new file mode 100644
> index ..6ffc151988a0
> --- /dev/null
> +++ b/Documentation/tests/kunit.rst
> @@ -0,0 +1,5 @@
> +Kunit tests
> +---
> +
> +.. include-test:: drivers/gpu/drm/tests/drm_buddy_test.c
> +
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c
> b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 09ee6f6af896..dd6c5afd6cd6 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -737,6 +737,18 @@ static int drm_buddy_suite_init(struct kunit_suite
> *suite)
> return 0;
> }
>
> +/**
> + * KTEST_SUITE: set of tests for drm buddy alloc
> + * Scope: drm subsystem
> + * Mega feature: drm
> + * Feature: buddy_alloc
> + *
> + * KTEST_TEST: drm_test_buddy_alloc_%s
> + * Description: Run DRM buddy allocation %arg[1] test
> + *
> + * arg[1].values: limit, range, optimistic, smoke, pathological
> + */
Hi!
This is such a cool patch series. I just have a few comments related to the
output.
In the html output the tests are listed as:
ktest@drm_buddy_test@…
I wonder if instead of using the file name of “drm_buddy_test” this could
possibly be the suite name, “drm_buddy”, as this is what users will call
when using kunit.py to run the tests. Although "drm_buddy_test" is also the
module name so I don't mind it too much. But in the future the file name
and module name are not guaranteed to be the same for other tests.
Most preferably, there would be a reference to the kunit suite name, file
name, and the module name.
This may be difficult to implement as these can all differ. I am currently
working on the KUnit Attribute framework which saves the module name and I
am thinking about also saving the file path as a future attribute. This
could be a helpful framework for the KUnit tests specifically.
I am not sure how easy it would be to access c objects/functions using this
system.
Finally, I was wondering if it is the intention to put a list of all kunit
tests that use this new feature into tests/kunit.rst or would this be
broken up in some way
Thanks!
-Rae
> +
> static struct kunit_case drm_buddy_tests[] = {
> KUNIT_CASE(drm_test_buddy_alloc_limit),
> KUNIT_CASE(drm_test_buddy_alloc_range),
> --
> 2.40.1
>
>
