[PATCH v2] selftests: Improve test output grammar, code style

[PATCH v2] selftests: Improve test output grammar, code style

[Hanne-Lotta Mäenpää]

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.

Signed-off-by: Hanne-Lotta Mäenpää <hannelotta@gmail.com>
---

Notes:
    v1 -> v2: Improved wording in RTC tests based on feedback from
    Alexandre Belloni <alexandre.belloni@bootlin.com>

 tools/testing/selftests/perf_events/watermark_signal.c |  7 ++++---
 tools/testing/selftests/rtc/rtctest.c                  | 10 +++++-----
 2 files changed, 9 insertions(+), 8 deletions(-)

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",
 			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);
 		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 */
 		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.");
 	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, &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);
 
 	return ret;
-- 
2.39.5

Re: [PATCH v2] selftests: Improve test output grammar, code style

[Shuah Khan]

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@gmail.com>
> ---
> 
> Notes:
>      v1 -> v2: Improved wording in RTC tests based on feedback from
>      Alexandre Belloni <alexandre.belloni@bootlin.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


> 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.

>   		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?

>   		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"

>   	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?

>   
>   	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.

>   
>   	return ret;

thanks,
-- Shuah

Re: [PATCH v2] selftests: Improve test output grammar, code style

[Hanne-Lotta Mäenpää]

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@gmail.com>
>> ---
>>
>> Notes:
>>      v1 -> v2: Improved wording in RTC tests based on feedback from
>>      Alexandre Belloni <alexandre.belloni@bootlin.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ää

Re: [PATCH v2] selftests: Improve test output grammar, code style

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 4591 bytes --]

On Sun, May 25, 2025 at 09:27:31PM +0300, Hanne-Lotta Mäenpää wrote:
> 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@gmail.com>
> > > ---
> > > 
> > > Notes:
> > >      v1 -> v2: Improved wording in RTC tests based on feedback from
> > >      Alexandre Belloni <alexandre.belloni@bootlin.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)?

Send both patches as a series.

> > 
> > >           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.

s/then/than/ typofix should be kept.

> 
> > 
> > >           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.

Disambiguate (like "alarms feature is 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.");
> > 
> > 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.

I would rather write the message as "Skipping test since the alarm has ..."

Thanks. 

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]