Message-ID: <ff076589-4a93-4e4c-8e85-77499e27898c@gmail.com>
Date: Sun, 25 May 2025 21:27:31 +0300
From: Hanne-Lotta Mäenpää <hannelotta@...il.com>
To: Shuah Khan <skhan@...uxfoundation.org>, shuah@...nel.org,
 alexandre.belloni@...tlin.com
Cc: linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rtc@...r.kernel.org
Subject: Re: [PATCH v2] selftests: Improve test output grammar, code style

Hello,

On 5/23/25 01:14, Shuah Khan wrote:
> On 5/16/25 02:42, Hanne-Lotta Mäenpää wrote:
>> Add small grammar fixes in perf events and Real Time Clock tests'
>> output messages.
>>
>> Include braces around a single if statement, when there are multiple
>> statements in the else branch, to align with the kernel coding style.
> 
> This patch combines several changes in one including combining changes
> to two tests.
> 
>>
>> Signed-off-by: Hanne-Lotta Mäenpää <hannelotta@...il.com>
>> ---
>>
>> Notes:
>>      v1 -> v2: Improved wording in RTC tests based on feedback from
>>      Alexandre Belloni <alexandre.belloni@...tlin.com>
>>
>>   tools/testing/selftests/perf_events/watermark_signal.c |  7 ++++---
>>   tools/testing/selftests/rtc/rtctest.c                  | 10 +++++-----
>>   2 files changed, 9 insertions(+), 8 deletions(-)
>>
> 
> Send separate patches for selftests/perf_events and selftests/rtc/rtctest.c

Sure, I can do that. If I split this patch into two, is it OK to send 
the other patch as a new one (without version history)? Or should I send 
both patches converted to a patch series (v3)?

> 
> 
>> diff --git a/tools/testing/selftests/perf_events/watermark_signal.c b/ 
>> tools/testing/selftests/perf_events/watermark_signal.c
>> index 49dc1e831174..6176afd4950b 100644
>> --- a/tools/testing/selftests/perf_events/watermark_signal.c
>> +++ b/tools/testing/selftests/perf_events/watermark_signal.c
>> @@ -65,8 +65,9 @@ TEST(watermark_signal)
>>       child = fork();
>>       EXPECT_GE(child, 0);
>> -    if (child == 0)
>> +    if (child == 0) {
>>           do_child();
>> +    }
>>       else if (child < 0) {
>>           perror("fork()");
>>           goto cleanup;
>> @@ -75,7 +76,7 @@ TEST(watermark_signal)
>>       if (waitpid(child, &child_status, WSTOPPED) != child ||
>>           !(WIFSTOPPED(child_status) && WSTOPSIG(child_status) == 
>> SIGSTOP)) {
>>           fprintf(stderr,
>> -            "failed to sycnhronize with child errno=%d status=%x\n",
>> +            "failed to synchronize with child errno=%d status=%x\n",
> 
> This change is good.
> 
>>               errno,
>>               child_status);
>>           goto cleanup;
>> @@ -84,7 +85,7 @@ TEST(watermark_signal)
>>       fd = syscall(__NR_perf_event_open, &attr, child, -1, -1,
>>                PERF_FLAG_FD_CLOEXEC);
>>       if (fd < 0) {
>> -        fprintf(stderr, "failed opening event %llx\n", attr.config);
>> +        fprintf(stderr, "failed to setup performance monitoring 
>> %llx\n", attr.config);
> 
> This change make it hard to understand what went wrong unlike the original
> message.

Okay, in that case I will leave out this change.

> 
>>           goto cleanup;
>>       }
>> diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/ 
>> selftests/rtc/rtctest.c
>> index be175c0e6ae3..930bf0ce4fa6 100644
>> --- a/tools/testing/selftests/rtc/rtctest.c
>> +++ b/tools/testing/selftests/rtc/rtctest.c
>> @@ -138,10 +138,10 @@ TEST_F_TIMEOUT(rtc, date_read_loop, 
>> READ_LOOP_DURATION_SEC + 2) {
>>           rtc_read = rtc_time_to_timestamp(&rtc_tm);
>>           /* Time should not go backwards */
>>           ASSERT_LE(prev_rtc_read, rtc_read);
>> -        /* Time should not increase more then 1s at a time */
>> +        /* Time should not increase more than 1s per read */
>>           ASSERT_GE(prev_rtc_read + 1, rtc_read);
>> -        /* Sleep 11ms to avoid killing / overheating the RTC */
>> +        /* Sleep 11ms to avoid overheating the RTC */
> 
> This change removes important information. What is the reason for this
> change?

Well, it is less verbose and still informative (avoiding overheating). I 
can leave out this change, though.

> 
>>           nanosleep_with_retries(READ_LOOP_SLEEP_MS * 1000000);
>>           prev_rtc_read = rtc_read;
>> @@ -236,7 +236,7 @@ TEST_F(rtc, alarm_alm_set) {
>>       if (alarm_state == RTC_ALARM_DISABLED)
>>           SKIP(return, "Skipping test since alarms are not supported.");
>>       if (alarm_state == RTC_ALARM_RES_MINUTE)
>> -        SKIP(return, "Skipping test since alarms has only minute 
>> granularity.");
>> +        SKIP(return, "Skipping test since alarm has only minute 
>> granularity.");
>>       rc = ioctl(self->fd, RTC_RD_TIME, &tm);
>>       ASSERT_NE(-1, rc);
>> @@ -306,7 +306,7 @@ TEST_F(rtc, alarm_wkalm_set) {
>>       if (alarm_state == RTC_ALARM_DISABLED)
>>           SKIP(return, "Skipping test since alarms are not supported.");
> 
> This one still says "alarms"

Yes, because "alarms are not supported" refers to alarms as a feature.

> 
>>       if (alarm_state == RTC_ALARM_RES_MINUTE)
>> -        SKIP(return, "Skipping test since alarms has only minute 
>> granularity.");
>> +        SKIP(return, "Skipping test since alarm has only minute 
>> granularity.");
> 
> Isn't "alarms" consistent with other messages?

Yes, plural "alarms" would be consistent with other messages, and when 
referring to them as a feature. The verb form should then change, either:

- alarm has ... OR
- alarms have ...

In the test, only one alarm is set - it makes sense to refer to it as 
singular. I received feedback regarding this from Alexandre, because I 
had plural form in the first version of this patch.

> 
>>       rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time);
>>       ASSERT_NE(-1, rc);
>> @@ -502,7 +502,7 @@ int main(int argc, char **argv)
>>       if (access(rtc_file, R_OK) == 0)
>>           ret = test_harness_run(argc, argv);
>>       else
>> -        ksft_exit_skip("[SKIP]: Cannot access rtc file %s - Exiting\n",
>> +        ksft_exit_skip("Cannot access RTC file %s - exiting\n",
>>                           rtc_file);
> 
> I don't see any reason for this change either.

The function ksft_exit_skip already prints the keyword SKIP. It does not 
need to be passed as an argument to the function. Without this change 
the test output shows a duplicate "SKIP":

# 1..0 # SKIP [SKIP]: Cannot access rtc file /dev/rtc0 - Exiting

With this change applied, the output reads:

# 1..0 # SKIP Cannot access RTC file /dev/rtc0 - exiting

The duplicate keyword is omitted.

It might make sense to have this change in its own patch or patch 
series, because there are a few other similar occurrences in the test 
outputs. What do you think?

> 
>>       return ret;
> 
> thanks,
> -- Shuah

Thank you so much!

Best regards,

Hanne-Lotta Mäenpää

Powered by blists - more mailing lists