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ää <hannelo...@gmail.com> > > > --- > > > > > > Notes: > > > v1 -> v2: Improved wording in RTC tests based on feedback from > > > Alexandre Belloni <alexandre.bell...@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
signature.asc
Description: PGP signature
