Should probably run Test262 against this polyfill

justingrant commented 2 years ago

There are almost 4000 Temporal tests in Test262. Some of them were never in the non-Test262 tests of proposal-temporal (the ones you're now running with Jest), so it'd probably be good for you to get additional test coverage from Test262.

Take a look at the npm run test262 script in https://github.com/js-temporal/temporal-polyfill/blob/main/package.json for some copypasta you can use to get Test262 running in this polyfill too. If you run into any trouble, feel free to tag me (@justingrant) or @ptomato or @12wrigja and we can help.

Note that Temporal's Test262 tests are slooooooooooow because the Test262 harness is inefficient. It takes about 5-10 mins to run all 3700+ tests. But if this polyfill passes both Jest and Test262 then that's a really good sign! 😄

arshaw commented 2 years ago

Thanks for letting me know. I will rig up the Tests262 tests and have them run alongside the demitasse ones.

LinusU commented 1 year ago

@arshaw did you make any progress on this? We have native BigInt support on our platform so I would like to take advantage of that, but don't feel comfortable switching over before this

If you want to I might be able to take a stab at this

justingrant commented 1 year ago

There's now over 5000 tests in Test262. It'd be good to get this repo hooked up to Test262.

FYI, there have been some improvements in Test262 performance over in https://github.com/tc39/proposal-temporal/blob/main/polyfill/runtest262.mjs which sped up test running a lot. Test runs can now finish in 1-2 mins in CI, and faster locally.

arshaw commented 1 year ago

Apologies, this was automatically closed when I merged the PR. reopening

arshaw commented 1 year ago

Hey @justingrant , do you have any suggestion how I might get this working with breakpoints (essentially the node --inspect option, paired with VSCode)? Right now, the test source code is grabbed as a string, so it'd be impossible for a debugger to work methinks.

BTW, this codebase now uses pnpm

12wrigja commented 1 year ago

You might want to use the pre-packaged runner here: https://www.npmjs.com/package/@js-temporal/temporal-test262-runner

In that repo, I added sourcemap URLs (in https://github.com/js-temporal/temporal-test262-runner/pull/10) to the test sources, so it should be possible to set breakpoints. I haven't tried doing this through vscode, but I have successfully done so via the Chrome DevTools.

arshaw commented 1 year ago

Just a quick update.

@12wrigja, I have the test runner working well now. It's brilliant, thank you. I added some minor features and might submit a PR eventually.

@justingrant and @LinusU, I've made significant progress on this. 87% of test are now passing:

  5403 passed
  815 failed
  0 passed unexpectedly
  64 expected failures

Most failures were related to input validation.

My WIP branch is test262-fixes. The code has become rather ugly and non-DRY, which I plan to optimize after all tests are passing.

justingrant commented 1 year ago

@arshaw That's great progress, and is really good to hear! Heads up: there's a large PR coming (hopefully the last big one before Stage 4) here: https://github.com/tc39/proposal-temporal/pull/2482. This PR replaces the timeZone property of ZonedDateTime and the calendar property of ZDT and Plain* types with timeZoneId/calendarId properties that are strings, and getTimeZone()/getCalendar() methods that return objects.

When this PR's accompanying Test262 tests are merged, likely in the next few weeks, (@ptomato should know more details) then you should expect another few hundred broken tests until your polyfill is revised to match.

Also a bunch of smaller normative changes from last week's TC39 meeting, plus a handful of PRs that aren't merged yet, should be landing soon. See https://github.com/tc39/proposal-temporal/pulls. Apologies for the churn! Thankfully we're coming down to the end of normative changes... likely very few (perhaps zero!) before Stage 4. It all depends on the feedback we get from JS engine implementers over the next few months.

FYI, it's also remotely possible that there may be one last big change (to remove nanoseconds from the API) but at this point this seems unlikely. Instead we'll probably end up imposing limits on the properties of Temporal.Duration, e.g. some values cannot be larger than MAX_SAFE_INTEGER. Stay tuned... we should have this resolved before the next TC39 meeting in the middle of May.

arshaw commented 1 year ago

I've made a lot of progress here. I've done a big refactor to fix some foundational issues. There's still a lot more work to do, but I've successfully ran the whole test suite. All tests are either passing, are failing with known bugs I've triaged, or are failing with minor intentionally deviations from the spec (which I plan on documenting). Test run:

6676 tests finished in 11.5 s
  5749 passed
  0 failed
  0 passed unexpectedly
  927 expected failures

The CI runs in the refactor-ci branch, example run here. Dev branch is refactor-scratch.

I'll update everyone as I make progress with the remaining failures.

@justingrant, I'm aware I'll probably need to pull in the latest test262 tests and adapt to new normative changes. I'm also aware of new normative changes in the PR pipeline.

justingrant commented 1 year ago

Great progress here! Thanks for the update.

arshaw commented 8 months ago

Hello @justingrant, v0.2.0 now has test262 tests passing to a satisfactory degree. Please see this section in the README: https://github.com/fullcalendar/temporal-polyfill/blob/v0.2.0/README.md#spec-compliance

And of course let me know if any feedback.

justingrant commented 8 months ago

Cool! I have a question about this:

Canonicalization of time zone IDs is simplified, leveraging the built-in Intl API.

Could you give a bit more detail about this simplification?

arshaw commented 8 months ago

Of course! temporal-polyfill uses the same technique as js-temporal/polyfill, which is: https://github.com/js-temporal/temporal-polyfill/blob/main/lib/ecmascript.ts#L3164

For the same reason:

// In the spec, GetCanonicalTimeZoneIdentifier is infallible and is always
// preceded by a call to IsAvailableTimeZoneName. However in the polyfill,
// we don't (yet) have a way to check if a time zone ID is valid without
// also canonicalizing it. So we combine both operations into one function,
// which will return the canonical ID if the ID is valid, and will throw
// if it's not.

Instead of doing this: https://github.com/tc39/proposal-temporal/blob/main/polyfill/lib/ecmascript.mjs#L2726C8-L2726C8

Figured this is the only feasible way for a polyfill.

It causes tests like this to fail: https://github.com/tc39/test262/blob/main/test/intl402/Temporal/ZonedDateTime/from/do-not-canonicalize-iana-identifiers.js

justingrant commented 8 months ago

js-temporal/polyfill is a bit out of date. When we have time to port the latest commits from the proposal repo, then js-temporal/polyfill will likely use the same logic as https://github.com/tc39/proposal-temporal/blob/main/polyfill/lib/ecmascript.mjs#L2726C8-L2726C8.

Out of curiosity, why do you not want to polyfill that behavior?

arshaw commented 8 months ago

Hi @justingrant, I've revised my opinion about whether this polyfill should canonicalize timeZone IDs. I mistakenly thought the reference implementation leveraged some sort of hardcoded timeZone ID database, but I now see it mainly uses string manipulations in tandem with some manual overrides. I'm confident this polyfill can represent that logic in a more compressed way, and become conformant. Will do that for the next release.

justingrant commented 8 months ago

revised my opinion about whether this polyfill should canonicalize timeZone IDs.

Cool, this is good to hear. Yeah, there's probably a more space-efficient way to do that polyfilling. Glad you're looking at how to do that.

One clarification: what the polyfill is doing is *not* canonicalizing IDs like Intl.DateTimeFormat does. For example, current Intl.DateTimeFormat on Chrome does this:

new Intl.DateTimeFormat("en", { timeZone: 'Asia/Kolkata' }).resolvedOptions().timeZone
// => Asia/Calcutta

But the Temporal spec does not change the user's ID, other than normalizing letter case:

Temporal.TimeZone.from('Asia/Kolkata').id
// => Asia/Kolkata
Temporal.TimeZone.from('Asia/Calcutta').id
// => Asia/Calcutta
Temporal.TimeZone.from('ASIA/Calcutta').id
// => Asia/Calcutta

Note that Temporal will make changes to Intl.DateTimeFormat so it will get the same behavior in a Temporal-native engine once they're released. But in the meantime this behavior must be polyfilled.

The case-normalization behavior in the spec exists as a perf optimization, so implementations don't have to store the user's input string as is (e.g. aSiA/cAlCutTa) but instead can store only the 10-bit index into an global array of ~600 time zone names. It's a nice win every time a Temporal.TimeZone is stored in RAM.

But in the meantime, a polyfill is needed to avoid mapping Asia/Kolkata to Asia/Calcutta (or vice versa on Firefox). The polyfill code you reference above is needed to do the case-normalization, as an alternative to having to ship a list of all 600 IDs in the polyfill. It may require updating in the future in the (very rare) case that a new ID is added that has a word that doesn't start with a capital letter and have all lower-case letters after that, like America/Port_of_Spain or Antacrtica/McMurdo.

If you're curious, more context about time zone ID canonicalization behavior is in https://github.com/tc39/proposal-canonical-tz, which was recently merged into Temporal after proposal-canonical-tz reached Stage 3.

arshaw commented 7 months ago

@justingrant, I was able to compress time zone ID normalization logic pretty well: https://github.com/fullcalendar/temporal-polyfill/blob/fad4d301bd000207a5d2ce9de566da8fc5d5bc76/packages/temporal-polyfill/src/internal/timeZoneId.ts#L60-L93

Released in https://github.com/fullcalendar/temporal-polyfill/releases/tag/v0.2.1

arshaw commented 7 months ago

@justingrant, in your opinion, are any of temporal-polyfill's intentional deviations from the spec considered "showstoppers", where you'd not recommend this polyfill? An updated summary of deviations:

The order, frequency, and exact arguments of how CalendarProtocol/TimeZoneProtocol methods are called. Only affects those writing custom Calendars/TimeZones. In many cases, temporal-polyfill has fewer calls.
Bugs we're sorting out on the proposal-temporal issue tracker.
Possibly the precision of how Duration::toString displays very large numbers, though looks like the test262 might have been updated recently. I'll check.

For a full list of test failures, see expected-failures.txt as well as some of the other files in that same directory.

I'm trying to understand if I should close this ticket or not.

PS- I haven't updated my test262 tests in a month. I plan to do that soon, and continuously update them thereafter.

justingrant commented 7 months ago

I'd suggest to leave it open until the latest round of normative changes land in the proposal repo and you have a chance to update this polyfill to align to them.

Then it should be clearer what's remaining.

BTW, if you've found what you think are more efficient algorithms and/or fewer user calls for some operations, that could be useful to the proposal itself. Fewer calls is good! Would you be willing to open issues over in the proposal repo for cases where you think you've found a more efficient version of some operations? This is a good way first to validate that your shortcuts are OK, and also for us to consider if we want to switch to your version.

arshaw commented 7 months ago

@justingrant, sure thing, I'll plan to become compliant with the latest normative changes and post tickets with more efficient algorithms sometime during the week of Feb 26 - Mar 1.

arshaw commented 7 months ago

@justingrant, tickets/PR's for fewer calls to user-code:

arshaw commented 7 months ago

There's also quite a few user-code reductions for all PlainYearMonth arithmetics (add/subtract/since/until) ... Calendar::fields and Calendar::dateFromFields is called very frequently. I'll make a PR very soon.

arshaw commented 7 months ago

New tracking issue I created for exploring reductions to user-code calls and overall DRY-ness and filesize reductions: https://github.com/tc39/proposal-temporal/issues/2793

fullcalendar / temporal-polyfill

Should probably run Test262 against this polyfill #3