In this series, test_bitmap is being converted to kunit test. Multiple
patches will make the review process smooth.

- Patch-1: Convert the tests in lib/test_bitmap.c to kunit
- Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
           configuration options
- Patch-3: Remove the bitmap.sh selftest

Muhammad Usama Anjum (3):
  bitmap: convert test_bitmap to KUnit test
  bitmap: Rename module
  selftests: lib: remove test_bitmap

 MAINTAINERS                           |   2 +-
 lib/Kconfig.debug                     |  15 +-
 lib/Makefile                          |   2 +-
 lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
 tools/testing/selftests/lib/Makefile  |   2 +-
 tools/testing/selftests/lib/bitmap.sh |   3 -
 tools/testing/selftests/lib/config    |   1 -
 7 files changed, 295 insertions(+), 354 deletions(-)
 rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
 delete mode 100755 tools/testing/selftests/lib/bitmap.sh

-- 
2.39.2

On 7/26/24 05:06, Muhammad Usama Anjum wrote:
> In this series, test_bitmap is being converted to kunit test. Multiple
> patches will make the review process smooth.
> 
> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>             configuration options
> - Patch-3: Remove the bitmap.sh selftest
> 
> Muhammad Usama Anjum (3):
>    bitmap: convert test_bitmap to KUnit test
>    bitmap: Rename module
>    selftests: lib: remove test_bitmap
> 
>   MAINTAINERS                           |   2 +-
>   lib/Kconfig.debug                     |  15 +-
>   lib/Makefile                          |   2 +-
>   lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>   tools/testing/selftests/lib/Makefile  |   2 +-
>   tools/testing/selftests/lib/bitmap.sh |   3 -
>   tools/testing/selftests/lib/config    |   1 -
>   7 files changed, 295 insertions(+), 354 deletions(-)
>   rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>   delete mode 100755 tools/testing/selftests/lib/bitmap.sh
> 

Can you tell me how this conversion helps?

It is removing the ability to run bitmap tests during boot.
It doesn't make sense to blindly convert all test under lib
to kunit - Nack on this change or any change that takes away
the ability to run tests and makes them dependent on kunit.

thanks,
-- Shuah

On 7/27/24 12:26 AM, Shuah Khan wrote:
> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
>> In this series, test_bitmap is being converted to kunit test. Multiple
>> patches will make the review process smooth.
>>
>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>>             configuration options
>> - Patch-3: Remove the bitmap.sh selftest
>>
>> Muhammad Usama Anjum (3):
>>    bitmap: convert test_bitmap to KUnit test
>>    bitmap: Rename module
>>    selftests: lib: remove test_bitmap
>>
>>   MAINTAINERS                           |   2 +-
>>   lib/Kconfig.debug                     |  15 +-
>>   lib/Makefile                          |   2 +-
>>   lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>>   tools/testing/selftests/lib/Makefile  |   2 +-
>>   tools/testing/selftests/lib/bitmap.sh |   3 -
>>   tools/testing/selftests/lib/config    |   1 -
>>   7 files changed, 295 insertions(+), 354 deletions(-)
>>   rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>>   delete mode 100755 tools/testing/selftests/lib/bitmap.sh
>>
> 
> Can you tell me how this conversion helps?
> 
> It is removing the ability to run bitmap tests during boot.
> It doesn't make sense to blindly convert all test under lib
> to kunit - Nack on this change or any change that takes away
> the ability to run tests and makes them dependent on kunit.
Let's discuss this on discussion thread [1].

[1]
https://lore.kernel.org/all/a3083ad4-e9dc-40da-bf57-8391bcd96a6c@collabora.com

> 
> thanks,
> -- Shuah

-- 
BR,
Muhammad Usama Anjum

On 7/29/24 02:29, Muhammad Usama Anjum wrote:
> On 7/27/24 12:26 AM, Shuah Khan wrote:
>> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
>>> In this series, test_bitmap is being converted to kunit test. Multiple
>>> patches will make the review process smooth.
>>>
>>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
>>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>>>              configuration options
>>> - Patch-3: Remove the bitmap.sh selftest
>>>
>>> Muhammad Usama Anjum (3):
>>>     bitmap: convert test_bitmap to KUnit test
>>>     bitmap: Rename module
>>>     selftests: lib: remove test_bitmap
>>>
>>>    MAINTAINERS                           |   2 +-
>>>    lib/Kconfig.debug                     |  15 +-
>>>    lib/Makefile                          |   2 +-
>>>    lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>>>    tools/testing/selftests/lib/Makefile  |   2 +-
>>>    tools/testing/selftests/lib/bitmap.sh |   3 -
>>>    tools/testing/selftests/lib/config    |   1 -
>>>    7 files changed, 295 insertions(+), 354 deletions(-)
>>>    rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>>>    delete mode 100755 tools/testing/selftests/lib/bitmap.sh
>>>
>>
>> Can you tell me how this conversion helps?
>>
>> It is removing the ability to run bitmap tests during boot.
>> It doesn't make sense to blindly convert all test under lib
>> to kunit - Nack on this change or any change that takes away
>> the ability to run tests and makes them dependent on kunit.
> Let's discuss this on discussion thread [1].

So - it doesn't mean that it is a done deal. Each patch will be
reviewed on individual basis. This test in particular clearly its
use-case right in the config which was deleted without understanding
it.

-config TEST_BITMAP
-	tristate "Test bitmap_*() family of functions at runtime"
-	help
-	  Enable this option to test the bitmap functions at boot.


This line above is the important piece of information which tells
you how the test is intended to be used.

1. You can enable this option and boot the kernel to check for regressions.
2. You can load the module on a running kernel to check for health.

Converting it to kunit drops support for these two use-cases which
are user-space regressions. You don't want to do that.
  
-
-	  If unsure, say N.
-
  config TEST_UUID
  	tristate "Test functions located in the uuid module at runtime"
  
@@ -2813,6 +2806,14 @@ config USERCOPY_KUNIT_TEST
  	  on the copy_to/from_user infrastructure, making sure basic
  	  user/kernel boundary testing is working.
  
+config BITMAP_KUNIT_TEST
+	tristate "KUnit Test for bitmap_*() family of functions"
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This builds the "bitmap_kunit" module that runs tests for
+	  bitmaps int the kernel making sure that there isn't any bug.

And this isn't complete even. I don'ty see tristate in here.

I responded to the thread that started this flurry of conversion activity
stating that converting tests without thinking through the use-cases
isn't what we want to do.

Reports aren't everything. The primary reason we have these tests is for
developers to be able to test. Reports can be improved and shouldn't
come at the expense of coverage and testing. Any patch that does that
will be NACKed.

thanks,
-- Shuah

On Tue, 30 Jul 2024 at 23:49, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/29/24 02:29, Muhammad Usama Anjum wrote:
> > On 7/27/24 12:26 AM, Shuah Khan wrote:
> >> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
> >>> In this series, test_bitmap is being converted to kunit test. Multiple
> >>> patches will make the review process smooth.
> >>>
> >>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
> >>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
> >>>              configuration options
> >>> - Patch-3: Remove the bitmap.sh selftest
> >>>
> >>> Muhammad Usama Anjum (3):
> >>>     bitmap: convert test_bitmap to KUnit test
> >>>     bitmap: Rename module
> >>>     selftests: lib: remove test_bitmap
> >>>
> >>>    MAINTAINERS                           |   2 +-
> >>>    lib/Kconfig.debug                     |  15 +-
> >>>    lib/Makefile                          |   2 +-
> >>>    lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
> >>>    tools/testing/selftests/lib/Makefile  |   2 +-
> >>>    tools/testing/selftests/lib/bitmap.sh |   3 -
> >>>    tools/testing/selftests/lib/config    |   1 -
> >>>    7 files changed, 295 insertions(+), 354 deletions(-)
> >>>    rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
> >>>    delete mode 100755 tools/testing/selftests/lib/bitmap.sh
> >>>
> >>
> >> Can you tell me how this conversion helps?
> >>
> >> It is removing the ability to run bitmap tests during boot.
> >> It doesn't make sense to blindly convert all test under lib
> >> to kunit - Nack on this change or any change that takes away
> >> the ability to run tests and makes them dependent on kunit.
> > Let's discuss this on discussion thread [1].
>
> So - it doesn't mean that it is a done deal. Each patch will be
> reviewed on individual basis. This test in particular clearly its
> use-case right in the config which was deleted without understanding
> it.
>
> -config TEST_BITMAP
> -       tristate "Test bitmap_*() family of functions at runtime"
> -       help
> -         Enable this option to test the bitmap functions at boot.
>
>
> This line above is the important piece of information which tells
> you how the test is intended to be used.
>
> 1. You can enable this option and boot the kernel to check for regressions.

Converting the test to KUnit _does not break this_. You can still
enable this option and boot the kernel to test for regressions if it's
a KUnit test.

> 2. You can load the module on a running kernel to check for health.

This is where the disagreement lies in my mind. While you can do this
with KUnit, the kernel does have to have been built with CONFIG_KUNIT
enabled (either built-in or as a module).

(Indeed, KUnit adds the ability to trigger a test at runtime even if
it's built-in, via debugfs, so it's possible to do so even if
CONFIG_MODULES is disabled. That's a pretty niche use-case, I admit,
but I'd guess that the bitmap functionailty breaking in a way which
disappears when CONFIG_KUNIT is enabled is as well.)

In general, though, my interpretation of "Test ... at runtime" in
config option descriptions has not precluded introducing the
CONFIG_KUNIT dependency. We already have several KUnit tests with that
in their description, several of which were ported to KUnit later.

>
> Converting it to kunit drops support for these two use-cases which
> are user-space regressions. You don't want to do that.
>
> -
> -         If unsure, say N.
> -
>   config TEST_UUID
>         tristate "Test functions located in the uuid module at runtime"
>
> @@ -2813,6 +2806,14 @@ config USERCOPY_KUNIT_TEST
>           on the copy_to/from_user infrastructure, making sure basic
>           user/kernel boundary testing is working.
>
> +config BITMAP_KUNIT_TEST
> +       tristate "KUnit Test for bitmap_*() family of functions"
> +       depends on KUNIT
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds the "bitmap_kunit" module that runs tests for
> +         bitmaps int the kernel making sure that there isn't any bug.
>
> And this isn't complete even. I don'ty see tristate in here.

(This looks like it's tristate to me?)

>
> I responded to the thread that started this flurry of conversion activity
> stating that converting tests without thinking through the use-cases
> isn't what we want to do.
>
> Reports aren't everything. The primary reason we have these tests is for
> developers to be able to test. Reports can be improved and shouldn't
> come at the expense of coverage and testing. Any patch that does that
> will be NACKed.
>
> thanks,
> -- Shuah

On 7/30/24 21:06, David Gow wrote:
> On Tue, 30 Jul 2024 at 23:49, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 7/29/24 02:29, Muhammad Usama Anjum wrote:
>>> On 7/27/24 12:26 AM, Shuah Khan wrote:
>>>> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
>>>>> In this series, test_bitmap is being converted to kunit test. Multiple
>>>>> patches will make the review process smooth.
>>>>>
>>>>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
>>>>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>>>>>               configuration options
>>>>> - Patch-3: Remove the bitmap.sh selftest
>>>>>
>>>>> Muhammad Usama Anjum (3):
>>>>>      bitmap: convert test_bitmap to KUnit test
>>>>>      bitmap: Rename module
>>>>>      selftests: lib: remove test_bitmap
>>>>>
>>>>>     MAINTAINERS                           |   2 +-
>>>>>     lib/Kconfig.debug                     |  15 +-
>>>>>     lib/Makefile                          |   2 +-
>>>>>     lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>>>>>     tools/testing/selftests/lib/Makefile  |   2 +-
>>>>>     tools/testing/selftests/lib/bitmap.sh |   3 -
>>>>>     tools/testing/selftests/lib/config    |   1 -
>>>>>     7 files changed, 295 insertions(+), 354 deletions(-)
>>>>>     rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>>>>>     delete mode 100755 tools/testing/selftests/lib/bitmap.sh
>>>>>
>>>>
>>>> Can you tell me how this conversion helps?
>>>>
>>>> It is removing the ability to run bitmap tests during boot.
>>>> It doesn't make sense to blindly convert all test under lib
>>>> to kunit - Nack on this change or any change that takes away
>>>> the ability to run tests and makes them dependent on kunit.
>>> Let's discuss this on discussion thread [1].
>>
>> So - it doesn't mean that it is a done deal. Each patch will be
>> reviewed on individual basis. This test in particular clearly its
>> use-case right in the config which was deleted without understanding
>> it.
>>
>> -config TEST_BITMAP
>> -       tristate "Test bitmap_*() family of functions at runtime"
>> -       help
>> -         Enable this option to test the bitmap functions at boot.
>>
>>
>> This line above is the important piece of information which tells
>> you how the test is intended to be used.
>>
>> 1. You can enable this option and boot the kernel to check for regressions.
> 
> Converting the test to KUnit _does not break this_. You can still
> enable this option and boot the kernel to test for regressions if it's
> a KUnit test.
> 
>> 2. You can load the module on a running kernel to check for health.
> 
> This is where the disagreement lies in my mind. While you can do this
> with KUnit, the kernel does have to have been built with CONFIG_KUNIT
> enabled (either built-in or as a module).

Right. It appears the confusion is coming in because "What's the problem
enabling CONFIG_KUNIT". The question is would you recommend enabling
CONFIG_KUNIT in distribution kernels - there is a confusion that
unit testing can be enabled in all environments.

The answer is no. Running tests should not require should not require
CONFIG_KUNIT - I would call this not disagreement, but not understanding
the role of unit (open box) vs closed box testing.

We want to users to be able to run as many tests possible without
needing to enable KUnit or other config options to verify kernels
they build. KUnit is a tool for kernel developers to verify - it
is a unit testing framework that is where it brings value.

It isn't a replacement for selftests and other tests. I am not happy
to seer tests that are supported as kselftests be changed to now
require KUNIT to be enabled.

thanks,
-- Shuah

On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote:
> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
> > In this series, test_bitmap is being converted to kunit test. Multiple
> > patches will make the review process smooth.
> > 
> > - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
> > - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
> >             configuration options
> > - Patch-3: Remove the bitmap.sh selftest
> > 
> > Muhammad Usama Anjum (3):
> >    bitmap: convert test_bitmap to KUnit test
> >    bitmap: Rename module
> >    selftests: lib: remove test_bitmap
> > 
> >   MAINTAINERS                           |   2 +-
> >   lib/Kconfig.debug                     |  15 +-
> >   lib/Makefile                          |   2 +-
> >   lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
> >   tools/testing/selftests/lib/Makefile  |   2 +-
> >   tools/testing/selftests/lib/bitmap.sh |   3 -
> >   tools/testing/selftests/lib/config    |   1 -
> >   7 files changed, 295 insertions(+), 354 deletions(-)
> >   rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
> >   delete mode 100755 tools/testing/selftests/lib/bitmap.sh
> > 
> 
> Can you tell me how this conversion helps?
> 
> It is removing the ability to run bitmap tests during boot.
> It doesn't make sense to blindly convert all test under lib
> to kunit - Nack on this change or any change that takes away
> the ability to run tests and makes them dependent on kunit.

Hi Muhammad,

In addition to Shuah's and John's reasoning. This patch wipes the
test history (git blame will point on you for most of the test),
breaks boot-time testing support, messes with config names and
usability, and drops kselftest support for ... exactly, what?

KUNIT engine here doesn't improve on readability, neither shorten
the test length, to my taste.

If you'd like to contribute to bitmaps testing - I'm all for that.
This is the very core and performance-sensitive piece of kernel,
and any extra-coverage is always welcome.

But I think the best way would be either adding new cases to the
existing test, or writing a new test, KUNIT-based, if you like.

Thanks,
Yury

On 7/27/24 11:10 PM, Yury Norov wrote:
> On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote:
>> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
>>> In this series, test_bitmap is being converted to kunit test. Multiple
>>> patches will make the review process smooth.
>>>
>>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
>>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>>>             configuration options
>>> - Patch-3: Remove the bitmap.sh selftest
>>>
>>> Muhammad Usama Anjum (3):
>>>    bitmap: convert test_bitmap to KUnit test
>>>    bitmap: Rename module
>>>    selftests: lib: remove test_bitmap
>>>
>>>   MAINTAINERS                           |   2 +-
>>>   lib/Kconfig.debug                     |  15 +-
>>>   lib/Makefile                          |   2 +-
>>>   lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>>>   tools/testing/selftests/lib/Makefile  |   2 +-
>>>   tools/testing/selftests/lib/bitmap.sh |   3 -
>>>   tools/testing/selftests/lib/config    |   1 -
>>>   7 files changed, 295 insertions(+), 354 deletions(-)
>>>   rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>>>   delete mode 100755 tools/testing/selftests/lib/bitmap.sh
>>>
>>
>> Can you tell me how this conversion helps?
>>
>> It is removing the ability to run bitmap tests during boot.
>> It doesn't make sense to blindly convert all test under lib
>> to kunit - Nack on this change or any change that takes away
>> the ability to run tests and makes them dependent on kunit.
> 
> Hi Muhammad,
> 
> In addition to Shuah's and John's reasoning. This patch wipes the
> test history (git blame will point on you for most of the test),
When files are renamed, their history isn't lost. We just need to use
--follow option with git log to get complete history[1].

> breaks boot-time testing support, messes with config names and
> usability, and drops kselftest support for ... exactly, what?
AFAIU the kselftest wasn't detected the test results that's why I started
thinking on which could be best way to detect if any failure happens in
this test. Triggering the test from kselftest doesn't grantee the test it
would pass every time until we check results. For this kind of in-kernel
testing, kunit is best suites. Please find earlier discussion [2].

> 
> KUNIT engine here doesn't improve on readability, neither shorten
> the test length, to my taste.
> 
> If you'd like to contribute to bitmaps testing - I'm all for that.
> This is the very core and performance-sensitive piece of kernel,
> and any extra-coverage is always welcome.
> 
> But I think the best way would be either adding new cases to the
> existing test, or writing a new test, KUNIT-based, if you like.
> 

[1] https://git-scm.com/docs/git-log#Documentation/git-log.txt---follow
[2]
https://lore.kernel.org/all/327831fb-47ab-4555-8f0b-19a8dbcaacd7@collabora.com

> Thanks,
> Yury
> 

-- 
BR,
Muhammad Usama Anjum

On 7/29/24 02:15, Muhammad Usama Anjum wrote:
> On 7/27/24 11:10 PM, Yury Norov wrote:
>> On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote:
>>> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
>>>> In this series, test_bitmap is being converted to kunit test. Multiple
>>>> patches will make the review process smooth.
>>>>
>>>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
>>>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
>>>>              configuration options
>>>> - Patch-3: Remove the bitmap.sh selftest
>>>>
>>>> Muhammad Usama Anjum (3):
>>>>     bitmap: convert test_bitmap to KUnit test
>>>>     bitmap: Rename module
>>>>     selftests: lib: remove test_bitmap
>>>>
>>>>    MAINTAINERS                           |   2 +-
>>>>    lib/Kconfig.debug                     |  15 +-
>>>>    lib/Makefile                          |   2 +-
>>>>    lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
>>>>    tools/testing/selftests/lib/Makefile  |   2 +-
>>>>    tools/testing/selftests/lib/bitmap.sh |   3 -
>>>>    tools/testing/selftests/lib/config    |   1 -
>>>>    7 files changed, 295 insertions(+), 354 deletions(-)
>>>>    rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
>>>>    delete mode 100755 tools/testing/selftests/lib/bitmap.sh
>>>>
>>>
>>> Can you tell me how this conversion helps?
>>>
>>> It is removing the ability to run bitmap tests during boot.
>>> It doesn't make sense to blindly convert all test under lib
>>> to kunit - Nack on this change or any change that takes away
>>> the ability to run tests and makes them dependent on kunit.
>>
>> Hi Muhammad,
>>
>> In addition to Shuah's and John's reasoning. This patch wipes the
>> test history (git blame will point on you for most of the test),
> When files are renamed, their history isn't lost. We just need to use
> --follow option with git log to get complete history[1].
> 
>> breaks boot-time testing support, messes with config names and
>> usability, and drops kselftest support for ... exactly, what?
> AFAIU the kselftest wasn't detected the test results that's why I started
> thinking on which could be best way to detect if any failure happens in
> this test. Triggering the test from kselftest doesn't grantee the test it
> would pass every time until we check results. For this kind of in-kernel
> testing, kunit is best suites. Please find earlier discussion [2].
> 

KUnit isn't idea for cases where people would want to check a subsystem
on a running kernel - KUnit covers some use-cases and kselftest covers
others.

What happens if we are debugging a problem that requires us to debug on
a running system? Please don't go converting kselftest into kunit without
understanding how these are intended to be used.

Yes kselftest results need to be looked at. Write a parser which can
be improved. What you are doing is reducing the coverage and talking
away the ability to debug and test on running system.

Fix what needs to be fixed instead of deleting tests.

>>
>> KUNIT engine here doesn't improve on readability, neither shorten
>> the test length, to my taste.
>>
>> If you'd like to contribute to bitmaps testing - I'm all for that.
>> This is the very core and performance-sensitive piece of kernel,
>> and any extra-coverage is always welcome.
>>

+1 on this. Add new tests and look at the reports.

>> But I think the best way would be either adding new cases to the
>> existing test, or writing a new test, KUNIT-based, if you like.
>>
> 

+1

As I mentioned in my earlier message, I am going to nack all patches
that convert existing selftests to kunit such as this one.

thanks,
-- Shuah

On Tue, 30 Jul 2024 at 23:39, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/29/24 02:15, Muhammad Usama Anjum wrote:
> > On 7/27/24 11:10 PM, Yury Norov wrote:
> >> On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote:
> >>> On 7/26/24 05:06, Muhammad Usama Anjum wrote:
> >>>> In this series, test_bitmap is being converted to kunit test. Multiple
> >>>> patches will make the review process smooth.
> >>>>
> >>>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit
> >>>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other
> >>>>              configuration options
> >>>> - Patch-3: Remove the bitmap.sh selftest
> >>>>
> >>>> Muhammad Usama Anjum (3):
> >>>>     bitmap: convert test_bitmap to KUnit test
> >>>>     bitmap: Rename module
> >>>>     selftests: lib: remove test_bitmap
> >>>>
> >>>>    MAINTAINERS                           |   2 +-
> >>>>    lib/Kconfig.debug                     |  15 +-
> >>>>    lib/Makefile                          |   2 +-
> >>>>    lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++--------------
> >>>>    tools/testing/selftests/lib/Makefile  |   2 +-
> >>>>    tools/testing/selftests/lib/bitmap.sh |   3 -
> >>>>    tools/testing/selftests/lib/config    |   1 -
> >>>>    7 files changed, 295 insertions(+), 354 deletions(-)
> >>>>    rename lib/{test_bitmap.c => bitmap_kunit.c} (70%)
> >>>>    delete mode 100755 tools/testing/selftests/lib/bitmap.sh
> >>>>
> >>>
> >>> Can you tell me how this conversion helps?
> >>>
> >>> It is removing the ability to run bitmap tests during boot.
> >>> It doesn't make sense to blindly convert all test under lib
> >>> to kunit - Nack on this change or any change that takes away
> >>> the ability to run tests and makes them dependent on kunit.
> >>
> >> Hi Muhammad,
> >>
> >> In addition to Shuah's and John's reasoning. This patch wipes the
> >> test history (git blame will point on you for most of the test),
> > When files are renamed, their history isn't lost. We just need to use
> > --follow option with git log to get complete history[1].
> >
> >> breaks boot-time testing support, messes with config names and
> >> usability, and drops kselftest support for ... exactly, what?
> > AFAIU the kselftest wasn't detected the test results that's why I started
> > thinking on which could be best way to detect if any failure happens in
> > this test. Triggering the test from kselftest doesn't grantee the test it
> > would pass every time until we check results. For this kind of in-kernel
> > testing, kunit is best suites. Please find earlier discussion [2].
> >
>
> KUnit isn't idea for cases where people would want to check a subsystem
> on a running kernel - KUnit covers some use-cases and kselftest covers
> others.

Thanks, Shuah. That clarifies a lot. So the use-case we're concerned about is:
- We have an existing running kernel, which has CONFIG_KUNIT=n.
- Something breaks, and we want to check (for example) test_bitmap.
- We can modprobe test_bitmap on the current kernel.

That's definitely something KUnit can't do at the moment (though the
workaround of always building with CONFIG_KUNIT=m is working for Red
Hat and Android, and seems the closest equivalent). Maybe we need to
work out a way for KUnit tests to handle this case more smoothly.
Because I don't think there's anything special about this test which
makes this use-case more likely than any of the other KUnit tests we
have.

> What happens if we are debugging a problem that requires us to debug on
> a running system? Please don't go converting kselftest into kunit without
> understanding how these are intended to be used.

I don't think it's clear that this _is_ how the test is intended to be
used: the config help text (and earlier messages in this thread) were
all about running tests at boot, which is distinct from "debugging a
problem on a running system". I think this is where I've been most
confused. We've been happy to assume the "testing at runtime" case is
against a kernel with test configs enabled, too.

When it comes to test_bitmap specifically, I can't find any difference
(save possibly for a couple of performance test functions) between it
and any of the other tests we've converted to or written with KUnit in
the past. I'd imagine (and correct me if I'm wrong) that it's pretty
rare for there to be an issue which will be caught by test_bitmap on a
running kernel, but not on a fresh boot / in a kernel with KUnit
enabled. So the "debug a running system" situation seems to me only
decrease coverage of some very rare situations. (Of course, that's
ignoring that running the module against the current kernel might be
most convenient, but that's a separate issue from coverage.)

> Yes kselftest results need to be looked at. Write a parser which can
> be improved. What you are doing is reducing the coverage and talking
> away the ability to debug and test on running system.
>
> Fix what needs to be fixed instead of deleting tests.
>
> >>
> >> KUNIT engine here doesn't improve on readability, neither shorten
> >> the test length, to my taste.
> >>
> >> If you'd like to contribute to bitmaps testing - I'm all for that.
> >> This is the very core and performance-sensitive piece of kernel,
> >> and any extra-coverage is always welcome.
> >>
>
> +1 on this. Add new tests and look at the reports.
>
> >> But I think the best way would be either adding new cases to the
> >> existing test, or writing a new test, KUNIT-based, if you like.
> >>
> >
>
> +1
>
> As I mentioned in my earlier message, I am going to nack all patches
> that convert existing selftests to kunit such as this one.

I should point out that we've already been porting a lot of tests in
lib/ to KUnit for years, sometimes with the help of the test author,
sometimes by encouraging it as a task for new contributors. And while
most of those are not actually selftests (though we may have ported
one or two), they differ from test_bitmap only in that there's a
selftest wrapper script. The authors and users of those tests have
thus far found this a worthwhile trade -- I've not heard any
complaints -- so I'd rather we leave this as a "pause" on new
conversions while we sort this out, rather than do something drastic
like revert existing conversion.

Personally, I'd still prefer these sorts of conversions to go ahead --
I think the benefits outweightthe costs -- but I can totally get
behind doing it on a case-per-case basis. I'd be disappointed if we
NACKed changes like this in the case where the authors and users of
that individual test would like it to be converted, though.

Regardless, I think there's more discussion to be had here, and I'll
look into what changes we can make either to KUnit or to the testing
documentation to make sure we're all on the same page.

-- David