[PATCH] kunit: time: Add faster unit test with shorter time range

[PATCH] kunit: time: Add faster unit test with shorter time range

[Guenter Roeck]

Commit a547c4ce10bd ("kunit: time: Mark test as slow using test
attributes") marked the time unit test as slow. This means it does not
run anymore if slow tests are disabled. This reduces test coverage and
is thus undesirable. At the same time, the test currently covers a range
of 160,000 years, which has limited value.

Add additional test case covering a total range of 1,600 years. This test
takes less than a second to run even on slow systems while still covering
twice the leap year calculation range of 400 years around the center date.
This test can run even with slow tests disabled.

Cc: Rae Moar <rmoar@google.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 kernel/time/time_test.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
index 3e5d422dd15c..15c6f3a5e73c 100644
--- a/kernel/time/time_test.c
+++ b/kernel/time/time_test.c
@@ -47,18 +47,18 @@ static void advance_date(long *year, int *month, int *mday, int *yday)
 }
 
 /*
- * Checks every day in a 160000 years interval centered at 1970-01-01
+ * Checks every day in a specified interval centered at 1970-01-01
  * against the expected result.
  */
-static void time64_to_tm_test_date_range(struct kunit *test)
+static void time64_to_tm_test_date_range(struct kunit *test, int years)
 {
 	/*
-	 * 80000 years	= (80000 / 400) * 400 years
-	 *		= (80000 / 400) * 146097 days
-	 *		= (80000 / 400) * 146097 * 86400 seconds
+	 * years	= (years / 400) * 400 years
+	 *		= (years / 400) * 146097 days
+	 *		= (years / 400) * 146097 * 86400 seconds
 	 */
-	time64_t total_secs = ((time64_t) 80000) / 400 * 146097 * 86400;
-	long year = 1970 - 80000;
+	time64_t total_secs = ((time64_t) years) / 400 * 146097 * 86400;
+	long year = 1970 - years;
 	int month = 1;
 	int mdday = 1;
 	int yday = 0;
@@ -85,8 +85,27 @@ static void time64_to_tm_test_date_range(struct kunit *test)
 	}
 }
 
+ /*
+  * Checks every day in a 1600 years interval centered at 1970-01-01
+  * against the expected result.
+  */
+static void time64_to_tm_test_date_range_1600(struct kunit *test)
+{
+	time64_to_tm_test_date_range(test, 800);
+}
+
+ /*
+  * Checks every day in a 160000 years interval centered at 1970-01-01
+  * against the expected result.
+  */
+static void time64_to_tm_test_date_range_160000(struct kunit *test)
+{
+	time64_to_tm_test_date_range(test, 80000);
+}
+
 static struct kunit_case time_test_cases[] = {
-	KUNIT_CASE_SLOW(time64_to_tm_test_date_range),
+	KUNIT_CASE(time64_to_tm_test_date_range_1600),
+	KUNIT_CASE_SLOW(time64_to_tm_test_date_range_160000),
 	{}
 };
 
-- 
2.39.2

Re: [PATCH] kunit: time: Add faster unit test with shorter time range

[Rae Moar]

On Wed, Mar 13, 2024 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Commit a547c4ce10bd ("kunit: time: Mark test as slow using test
> attributes") marked the time unit test as slow. This means it does not
> run anymore if slow tests are disabled. This reduces test coverage and
> is thus undesirable. At the same time, the test currently covers a range
> of 160,000 years, which has limited value.
>
> Add additional test case covering a total range of 1,600 years. This test
> takes less than a second to run even on slow systems while still covering
> twice the leap year calculation range of 400 years around the center date.
> This test can run even with slow tests disabled.

Hello!

I really like this addition of another time range test. This looks good to me.

Thanks!
-Rae

Reviewed-by: Rae Moar <rmoar@google.com>

>
> Cc: Rae Moar <rmoar@google.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  kernel/time/time_test.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
> index 3e5d422dd15c..15c6f3a5e73c 100644
> --- a/kernel/time/time_test.c
> +++ b/kernel/time/time_test.c
> @@ -47,18 +47,18 @@ static void advance_date(long *year, int *month, int *mday, int *yday)
>  }
>
>  /*
> - * Checks every day in a 160000 years interval centered at 1970-01-01
> + * Checks every day in a specified interval centered at 1970-01-01
>   * against the expected result.
>   */
> -static void time64_to_tm_test_date_range(struct kunit *test)
> +static void time64_to_tm_test_date_range(struct kunit *test, int years)
>  {
>         /*
> -        * 80000 years  = (80000 / 400) * 400 years
> -        *              = (80000 / 400) * 146097 days
> -        *              = (80000 / 400) * 146097 * 86400 seconds
> +        * years        = (years / 400) * 400 years

This is tiny but if there is another version, I find this comment a
bit confusing. Could you change this to maybe be "total seconds ="
instead of "years =" because years is used as a unit on the right side
of the equation?

> +        *              = (years / 400) * 146097 days
> +        *              = (years / 400) * 146097 * 86400 seconds
>          */
> -       time64_t total_secs = ((time64_t) 80000) / 400 * 146097 * 86400;
> -       long year = 1970 - 80000;
> +       time64_t total_secs = ((time64_t) years) / 400 * 146097 * 86400;
> +       long year = 1970 - years;
>         int month = 1;
>         int mdday = 1;
>         int yday = 0;
> @@ -85,8 +85,27 @@ static void time64_to_tm_test_date_range(struct kunit *test)
>         }
>  }
>
> + /*
> +  * Checks every day in a 1600 years interval centered at 1970-01-01
> +  * against the expected result.
> +  */
> +static void time64_to_tm_test_date_range_1600(struct kunit *test)
> +{
> +       time64_to_tm_test_date_range(test, 800);
> +}
> +
> + /*
> +  * Checks every day in a 160000 years interval centered at 1970-01-01
> +  * against the expected result.
> +  */
> +static void time64_to_tm_test_date_range_160000(struct kunit *test)
> +{
> +       time64_to_tm_test_date_range(test, 80000);
> +}
> +
>  static struct kunit_case time_test_cases[] = {
> -       KUNIT_CASE_SLOW(time64_to_tm_test_date_range),
> +       KUNIT_CASE(time64_to_tm_test_date_range_1600),
> +       KUNIT_CASE_SLOW(time64_to_tm_test_date_range_160000),
>         {}
>  };
>
> --
> 2.39.2
>

Re: [PATCH] kunit: time: Add faster unit test with shorter time range

[Guenter Roeck]

On 3/14/24 12:05, Rae Moar wrote:
> On Wed, Mar 13, 2024 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Commit a547c4ce10bd ("kunit: time: Mark test as slow using test
>> attributes") marked the time unit test as slow. This means it does not
>> run anymore if slow tests are disabled. This reduces test coverage and
>> is thus undesirable. At the same time, the test currently covers a range
>> of 160,000 years, which has limited value.
>>
>> Add additional test case covering a total range of 1,600 years. This test
>> takes less than a second to run even on slow systems while still covering
>> twice the leap year calculation range of 400 years around the center date.
>> This test can run even with slow tests disabled.
> 
> Hello!
> 
> I really like this addition of another time range test. This looks good to me.
> 
> Thanks!
> -Rae
> 
> Reviewed-by: Rae Moar <rmoar@google.com>
> 
>>
>> Cc: Rae Moar <rmoar@google.com>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   kernel/time/time_test.c | 35 +++++++++++++++++++++++++++--------
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
>> index 3e5d422dd15c..15c6f3a5e73c 100644
>> --- a/kernel/time/time_test.c
>> +++ b/kernel/time/time_test.c
>> @@ -47,18 +47,18 @@ static void advance_date(long *year, int *month, int *mday, int *yday)
>>   }
>>
>>   /*
>> - * Checks every day in a 160000 years interval centered at 1970-01-01
>> + * Checks every day in a specified interval centered at 1970-01-01
>>    * against the expected result.
>>    */
>> -static void time64_to_tm_test_date_range(struct kunit *test)
>> +static void time64_to_tm_test_date_range(struct kunit *test, int years)
>>   {
>>          /*
>> -        * 80000 years  = (80000 / 400) * 400 years
>> -        *              = (80000 / 400) * 146097 days
>> -        *              = (80000 / 400) * 146097 * 86400 seconds
>> +        * years        = (years / 400) * 400 years
> 
> This is tiny but if there is another version, I find this comment a
> bit confusing. Could you change this to maybe be "total seconds ="
> instead of "years =" because years is used as a unit on the right side
> of the equation?
> 

Good point. "total seconds" might be just as confusing, though.
How about "total range", "time range", or just "range" ?

Thanks,
Guenter

Re: [PATCH] kunit: time: Add faster unit test with shorter time range

[Rae Moar]

On Thu, Mar 14, 2024 at 3:30 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/14/24 12:05, Rae Moar wrote:
> > On Wed, Mar 13, 2024 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> Commit a547c4ce10bd ("kunit: time: Mark test as slow using test
> >> attributes") marked the time unit test as slow. This means it does not
> >> run anymore if slow tests are disabled. This reduces test coverage and
> >> is thus undesirable. At the same time, the test currently covers a range
> >> of 160,000 years, which has limited value.
> >>
> >> Add additional test case covering a total range of 1,600 years. This test
> >> takes less than a second to run even on slow systems while still covering
> >> twice the leap year calculation range of 400 years around the center date.
> >> This test can run even with slow tests disabled.
> >
> > Hello!
> >
> > I really like this addition of another time range test. This looks good to me.
> >
> > Thanks!
> > -Rae
> >
> > Reviewed-by: Rae Moar <rmoar@google.com>
> >
> >>
> >> Cc: Rae Moar <rmoar@google.com>
> >> Cc: Shuah Khan <skhan@linuxfoundation.org>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >>   kernel/time/time_test.c | 35 +++++++++++++++++++++++++++--------
> >>   1 file changed, 27 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
> >> index 3e5d422dd15c..15c6f3a5e73c 100644
> >> --- a/kernel/time/time_test.c
> >> +++ b/kernel/time/time_test.c
> >> @@ -47,18 +47,18 @@ static void advance_date(long *year, int *month, int *mday, int *yday)
> >>   }
> >>
> >>   /*
> >> - * Checks every day in a 160000 years interval centered at 1970-01-01
> >> + * Checks every day in a specified interval centered at 1970-01-01
> >>    * against the expected result.
> >>    */
> >> -static void time64_to_tm_test_date_range(struct kunit *test)
> >> +static void time64_to_tm_test_date_range(struct kunit *test, int years)
> >>   {
> >>          /*
> >> -        * 80000 years  = (80000 / 400) * 400 years
> >> -        *              = (80000 / 400) * 146097 days
> >> -        *              = (80000 / 400) * 146097 * 86400 seconds
> >> +        * years        = (years / 400) * 400 years
> >
> > This is tiny but if there is another version, I find this comment a
> > bit confusing. Could you change this to maybe be "total seconds ="
> > instead of "years =" because years is used as a unit on the right side
> > of the equation?
> >
>
> Good point. "total seconds" might be just as confusing, though.
> How about "total range", "time range", or just "range" ?
>

I see that. Any of those options look fine to me, maybe just "range"?
Whatever you think is best of those.

Thanks!
-Rae

> Thanks,
> Guenter
>

Re: [PATCH] kunit: time: Add faster unit test with shorter time range

[Guenter Roeck]

On 3/14/24 12:44, Rae Moar wrote:
> On Thu, Mar 14, 2024 at 3:30 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 3/14/24 12:05, Rae Moar wrote:
>>> On Wed, Mar 13, 2024 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Commit a547c4ce10bd ("kunit: time: Mark test as slow using test
>>>> attributes") marked the time unit test as slow. This means it does not
>>>> run anymore if slow tests are disabled. This reduces test coverage and
>>>> is thus undesirable. At the same time, the test currently covers a range
>>>> of 160,000 years, which has limited value.
>>>>
>>>> Add additional test case covering a total range of 1,600 years. This test
>>>> takes less than a second to run even on slow systems while still covering
>>>> twice the leap year calculation range of 400 years around the center date.
>>>> This test can run even with slow tests disabled.
>>>
>>> Hello!
>>>
>>> I really like this addition of another time range test. This looks good to me.
>>>
>>> Thanks!
>>> -Rae
>>>
>>> Reviewed-by: Rae Moar <rmoar@google.com>
>>>
>>>>
>>>> Cc: Rae Moar <rmoar@google.com>
>>>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>    kernel/time/time_test.c | 35 +++++++++++++++++++++++++++--------
>>>>    1 file changed, 27 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
>>>> index 3e5d422dd15c..15c6f3a5e73c 100644
>>>> --- a/kernel/time/time_test.c
>>>> +++ b/kernel/time/time_test.c
>>>> @@ -47,18 +47,18 @@ static void advance_date(long *year, int *month, int *mday, int *yday)
>>>>    }
>>>>
>>>>    /*
>>>> - * Checks every day in a 160000 years interval centered at 1970-01-01
>>>> + * Checks every day in a specified interval centered at 1970-01-01
>>>>     * against the expected result.
>>>>     */
>>>> -static void time64_to_tm_test_date_range(struct kunit *test)
>>>> +static void time64_to_tm_test_date_range(struct kunit *test, int years)
>>>>    {
>>>>           /*
>>>> -        * 80000 years  = (80000 / 400) * 400 years
>>>> -        *              = (80000 / 400) * 146097 days
>>>> -        *              = (80000 / 400) * 146097 * 86400 seconds
>>>> +        * years        = (years / 400) * 400 years
>>>
>>> This is tiny but if there is another version, I find this comment a
>>> bit confusing. Could you change this to maybe be "total seconds ="
>>> instead of "years =" because years is used as a unit on the right side
>>> of the equation?
>>>
>>
>> Good point. "total seconds" might be just as confusing, though.
>> How about "total range", "time range", or just "range" ?
>>
> 
> I see that. Any of those options look fine to me, maybe just "range"?
> Whatever you think is best of those.
> 

Let's just use "range". I'll wait a couple of days for additional feedback before
submitting v2.

Thanks,
Guenter