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;
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 */
/* 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
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ää
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.
linux-kselftest-mirror@lists.linaro.org
-
Bagas Sanjaya -
Hanne-Lotta Mäenpää -
Shuah Khan