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.