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

[Bagas Sanjaya <bagasdotme@gmail.com>]

[-- 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 --]