Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

martinpitt · 2024-06-24T21:02:54Z

This is a prerequisite for #20653 and also fixes an important bug.

martinpitt · 2024-06-25T06:16:07Z

I completed the unit tests, and hold and below, they found a bug! Fix added.

Now I just need to sort out the type checker.

allisonkarlitskaya · 2024-06-25T06:47:43Z

I completed the unit tests, and hold and below, they found a bug! Fix added.

I think you mean lo and behold :)

martinpitt · 2024-06-25T07:15:21Z

This unfortunately "affects" just about every test, so this exposes all our flakes. /me leans on retry button

allisonkarlitskaya

I'm personally insulted that you didn't choose America/Toronto in your manual testing.

Is there a way we can whack the TZ of the browser from inside so that we can get those cases under regression tests? Nobody is ever going to run that command ever again.

Otherwise, looks good except for the egregious use of an indexed access type.

allisonkarlitskaya · 2024-06-26T06:24:17Z

pkg/lib/cockpit-components-shutdown.jsx

@@ -31,7 +31,7 @@ import { DatePicker } from "@patternfly/react-core/dist/esm/components/DatePicke
 import { TimePicker } from "@patternfly/react-core/dist/esm/components/TimePicker/index.js";

 import { ServerTime } from 'serverTime.js';
-import * as timeformat from "timeformat.js";
+import * as timeformat from "timeformat";


We added a rule to Cockpit Files to forbid file extension on imports.....

Topic for another PR :)

allisonkarlitskaya · 2024-06-26T06:25:44Z

pkg/lib/timeformat.ts

@@ -8,47 +8,60 @@ import { parse, formatDistanceToNow } from 'date-fns';
 import * as locales from 'date-fns/locale';

 // this needs to be dynamic, as some pages don't initialize cockpit.language right away
-export const dateFormatLang = () => cockpit.language.replace('_', '-');
+export const dateFormatLang = (): string => cockpit.language.replace('_', '-');


Was it necessary to annotate the return type?

In general, return types can be omitted, and I usually do that. On the other hand, I've seen noises from "smart people" (including the TypeScript team) which suggest that type inference in other parts of the program will be faster if you explicitly annotate...

This isn't a request for change — just random musings.

I don't think it was necessary -- but if we do this, IMHO we should explicitly type all external API, to make it more self-documenting and avoid ambiguity. I like the python mypy approach to require types in declarations, even if inference could figure them out.

I'm going to start writing explicit return types from now on.

allisonkarlitskaya · 2024-06-26T06:29:11Z

pkg/lib/timeformat.ts

+const dateFnsLocale = (): any => (locales as any)[dateFormatLangDateFns()];
+
+type DTFOptions = {
+    [key: string]: string;


I think that's spelled Intl.DateTimeFormatOptions.

In general it's helpful to go looking inside of node_modules/typescript/lib/ or node_modules/@types to see what this thing is "officially" called, and use that.

Thanks! Fixed.

allisonkarlitskaya · 2024-06-26T06:30:06Z

pkg/lib/timeformat.ts

    if (cockpit.language == "en") return "enUS";
    else return cockpit.language.replace('_', '');
 };

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+const dateFnsLocale = (): any => (locales as any)[dateFormatLangDateFns()];


naja... I guess that part is going away soon...

allisonkarlitskaya · 2024-06-26T06:43:01Z

pkg/lib/timeformat.ts

+// 07:41 AM
+export const time = (t: Time): string => formatter({ timeStyle: "short" }).format(t);
+// 7:41:26 AM
+export const timeSeconds = (t: Time): string => formatter({ timeStyle: "medium" }).format(t);


I demand coverage!!

allisonkarlitskaya · 2024-06-26T06:43:22Z

pkg/lib/timeformat.ts

+// 07:41 AM
+export const time = (t: Time): string => formatter({ timeStyle: "short" }).format(t);
+// 7:41:26 AM
+export const timeSeconds = (t: Time): string => formatter({ timeStyle: "medium" }).format(t);


... or the 🪓

allisonkarlitskaya · 2024-06-26T06:44:21Z

pkg/base1/test-timeformat.ts

+    assert.equal(timeformat.distanceToNow(now - 62 * DAY), "2 Monate");
+});
+
+QUnit.test("firstDayOfWeek", assert => {


ah right. It would be nice if our coverage reporting also covered the QUnit runs...

martinpitt · 2024-06-26T06:58:00Z

pkg/lib/timeformat.ts

+const first_dow_sun = ['en', 'ja', 'ko', 'pt', 'pt_BR', 'sv', 'zh_CN', 'zh_TW'];
+
+export function firstDayOfWeek(): number {
+    return first_dow_sun.indexOf(cockpit.language) >= 0 ? 0 : 1;


that line is covered, just not both cases -- we dont' test the shutdown time or timer create dialog in non-English. But again, qunit test covers this.

We are about to rename it to *.ts.

Turn the optional `addSuffix` parameter of `distanceToNow()` into a proper boolean, to match `formatDistanceToNow()`'s signature. We have to apply an "any" crowbar for the "magic" locale import. But that's not exposed in the API, just an internal implementation detail.

Pay special attention to the relative date formatting, as we want to replace it (cockpit-project#20653). We don't need to replicate the exact behaviour, but should have some coverage. This finds a bug in parseShortDate(). Add a comment and skip the test.

martinpitt · 2024-06-26T07:26:08Z

Is there a way we can whack the TZ of the browser from inside so that we can get those cases under regression tests?

Not from within test-timeformat.ts, as they all run in the same browser instance. There's a CDP call, but we don't have access to that from inside a QUnit test.

A possible entry point would be in test/pytest/test_browser.py , it could run the timeformat test in separate test-server/browser runs. It's a bit ad-hoc, but I agree it'd be worth it. I did that now, and it reproduces the bug very nicely (and confirms the fix). And yes, it uses A/Toronto now 😁

Do the "strip off time and set to midnight" step already for the reference Date input of parse(). Substracting it afterwards previously caused the returned date to be off by one day for time zones other than UTC. Add `test_timeformat_timezones()` pytest to cover various time zones.

allisonkarlitskaya

Nice little piece of work. Thanks!

allisonkarlitskaya · 2024-06-26T08:08:32Z

test/pytest/test_browser.py

+
+
+# run test-timeformat.ts in different time zones: west/UTC/east
+@pytest.mark.parametrize('tz', ['America/Toronto', 'Europe/London', 'UTC', 'Europe/Berlin', 'Australia/Sydney'])


Cool.

test-timeformat is also going to run from the normal test_browser() path, but I guess that's good: then we get the version without explicit TZ set. It also means that pytest -k browser will still hit it (at least for the one case) but won't get bogged down with having to do the exhausting checking.

Right, I deliberately didn't ignore it from test_browser(), it's also nice to test in your current time zone.

allisonkarlitskaya · 2024-06-26T08:09:48Z

pkg/lib/timeformat.ts

@@ -8,47 +8,60 @@ import { parse, formatDistanceToNow } from 'date-fns';
 import * as locales from 'date-fns/locale';

 // this needs to be dynamic, as some pages don't initialize cockpit.language right away
-export const dateFormatLang = () => cockpit.language.replace('_', '-');
+export const dateFormatLang = (): string => cockpit.language.replace('_', '-');


I'm going to start writing explicit return types from now on.

allisonkarlitskaya · 2024-06-26T08:10:17Z

pkg/lib/timeformat.ts

 // general Intl.DateTimeFormat formatter object
-export const formatter = options => new Intl.DateTimeFormat(dateFormatLang(), options);
+export const formatter = (options?: Intl.DateTimeFormatOptions) => new Intl.DateTimeFormat(dateFormatLang(), options);


Much better, thanks!

cockpituous · 2024-06-26T08:54:37Z

pkg/lib/timeformat.ts

+// 07:41 AM
+export const time = (t: Time): string => formatter({ timeStyle: "short" }).format(t);
+// 7:41:26 AM
+export const timeSeconds = (t: Time): string => formatter({ timeStyle: "medium" }).format(t);


This added line is not executed by any test.

cockpituous · 2024-06-26T08:54:37Z

pkg/lib/timeformat.ts

+const first_dow_sun = ['en', 'ja', 'ko', 'pt', 'pt_BR', 'sv', 'zh_CN', 'zh_TW'];
+
+export function firstDayOfWeek(): number {
+    return first_dow_sun.indexOf(cockpit.language) >= 0 ? 0 : 1;


This added line is not executed by any test.

martinpitt added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jun 24, 2024

martinpitt mentioned this pull request Jun 24, 2024

Look into dropping date-fns #20653

Closed

martinpitt force-pushed the time-tests branch from ceeadbc to 00533a4 Compare June 25, 2024 06:15

martinpitt force-pushed the time-tests branch from 00533a4 to 3668196 Compare June 25, 2024 06:20

martinpitt removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jun 25, 2024

martinpitt changed the title ~~Type-annotate and unit-test timeformat~~ Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate( Jun 25, 2024

martinpitt changed the title ~~Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate(~~ Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() Jun 25, 2024

martinpitt force-pushed the time-tests branch 2 times, most recently from 3054bc5 to 803f048 Compare June 25, 2024 06:34

martinpitt marked this pull request as ready for review June 25, 2024 06:35

martinpitt requested review from jelly and allisonkarlitskaya June 25, 2024 06:35

martinpitt added the release-blocker Targetted for next release label Jun 25, 2024

allisonkarlitskaya requested changes Jun 26, 2024

View reviewed changes

martinpitt commented Jun 26, 2024

View reviewed changes

martinpitt added 3 commits June 26, 2024 09:06

pkg: Drop extension in timeformat imports

c70afc3

We are about to rename it to *.ts.

martinpitt force-pushed the time-tests branch from 803f048 to fbdda0e Compare June 26, 2024 07:26

martinpitt requested a review from allisonkarlitskaya June 26, 2024 07:26

This comment was marked as resolved.

Sign in to view

martinpitt force-pushed the time-tests branch from fbdda0e to d8cf4f4 Compare June 26, 2024 07:46

allisonkarlitskaya approved these changes Jun 26, 2024

View reviewed changes

cockpituous reviewed Jun 26, 2024

View reviewed changes

martinpitt merged commit 81bc367 into cockpit-project:main Jun 26, 2024
81 of 82 checks passed

martinpitt deleted the time-tests branch June 26, 2024 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

martinpitt commented Jun 24, 2024 •

edited

Loading

martinpitt commented Jun 25, 2024

allisonkarlitskaya commented Jun 25, 2024

martinpitt commented Jun 25, 2024

allisonkarlitskaya left a comment

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

martinpitt Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

martinpitt Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

martinpitt Jun 26, 2024

martinpitt commented Jun 26, 2024

This comment was marked as resolved.

allisonkarlitskaya left a comment

allisonkarlitskaya Jun 26, 2024

martinpitt Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

allisonkarlitskaya Jun 26, 2024

cockpituous Jun 26, 2024

cockpituous Jun 26, 2024



		# run test-timeformat.ts in different time zones: west/UTC/east
		@pytest.mark.parametrize('tz', ['America/Toronto', 'Europe/London', 'UTC', 'Europe/Berlin', 'Australia/Sydney'])

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

Conversation

martinpitt commented Jun 24, 2024 • edited Loading

martinpitt commented Jun 25, 2024

allisonkarlitskaya commented Jun 25, 2024

martinpitt commented Jun 25, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Jun 26, 2024

This comment was marked as resolved.

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Jun 24, 2024 •

edited

Loading