Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move existing Temporal tests from tc39/proposal-temporal #3223

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Sep 29, 2021

As discussed in #3002, this moves all the in-progress Temporal tests from tc39/proposal-temporal into here, so that it's more accessible to implementors. Further development is intended to happen here, as these tests will be removed from tc39/proposal-temporal (though they will still be executed using a git submodule).

ptomato added a commit to tc39/proposal-temporal that referenced this pull request Sep 29, 2021
Instead of an in-tree checkout use a git submodule of test262 so we can
pin the tests to a particular commit and pull in updates when we are
ready for them.

Requires some changes in various paths.

(NOTE: This is currently pinned to ptomato/test262's temporal-tests
branch. Needs to be updated to point to tc39/test262's main branch once
tc39/test262#3223 is merged.)
ptomato added a commit to tc39/proposal-temporal that referenced this pull request Sep 29, 2021
Instead of an in-tree checkout use a git submodule of test262 so we can
pin the tests to a particular commit and pull in updates when we are
ready for them.

Requires some changes in various paths.

(NOTE: This is currently pinned to ptomato/test262's temporal-tests
branch. Needs to be updated to point to tc39/test262's main branch once
tc39/test262#3223 is merged.)
@rwaldron
Copy link
Contributor

@ptomato just a heads up that including console.warn in your polyfill code means we can't run the tests in hosts like jsc or v8.

Copy link
Contributor

@rwaldron rwaldron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptomato I removed the console.warns from the polyfill and ran all of these against that in JSC. The following tests fail in both strict and default execution. Can you either fix the failures or provide clarity (ie. known failure for whatever reason)? Thanks!

FAIL test/built-ins/Temporal/Calendar/prototype/fields/long-input.js
  Expected no error, got RangeError: invalid field name garbage 1  

FAIL test/built-ins/Temporal/Duration/compare/calendar-dateadd-called-with-options-undefined.js
  dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true

FAIL test/built-ins/Temporal/Duration/compare/relativeto-string-datetime.js
  date-time + IANA annotation is a zoned relativeTo Expected SameValue(«-1», «0») to be true

FAIL test/built-ins/Temporal/Duration/prototype/round/calendar-dateadd-called-with-options-undefined.js
  dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true

FAIL test/built-ins/Temporal/Duration/prototype/total/calendar-dateadd-called-with-options-undefined.js
  dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true

FAIL test/built-ins/Temporal/ZonedDateTime/prototype/since/calendar-dateadd-called-with-options-undefined.js
  dateAdd shouldn't be called with options Expected SameValue(«[object Object]», «undefined») to be true

FAIL test/built-ins/Temporal/ZonedDateTime/prototype/toLocaleString/options-conflict.js
  Expected no error, got TypeError: dateStyle and timeStyle may not be used with other DateTimeFormat options

FAIL test/built-ins/Temporal/ZonedDateTime/prototype/until/calendar-dateadd-called-with-options-undefined.js
  Expected no error, got RangeError: mixed-sign values not allowed as duration fields

@jugglinmike
Copy link
Contributor

@rwaldron Philip would know better, but I believe the project's build262 script creates a version that's more suitable for use here.

@ptomato
Copy link
Contributor Author

ptomato commented Sep 30, 2021

@rwaldron Thanks for the quick review!

For the console.warn, the way we run the tests against the polyfill is TEST262=1 npm run build to build a script.js without the console.warn, and then use that as the prelude in test262-harness. (I think you could also run it the same way you've been doing, but set globalThis.__test262__ = true before importing the polyfill.)

Here's what's going on with each of those failures:

This copies over the tests that previously existed in the
tc39/proposal-temporal repository.

For context, see thread starting at:
tc39#3002 (comment)

In service of tc39#3002
There were a few tests already in the tree that overlapped ones that I
added in the previous commit. I've consolidated these and taken
information from the deleted ones where applicable, and improved on the
autogenerated assertion messages.
@ptomato
Copy link
Contributor Author

ptomato commented Sep 30, 2021

@rwaldron I've removed the tests that were skipped in the proposal repo now. That leaves two failures; one addressed by the existing PR.

The other I've tracked down to a difference in JSC and v8:
v8 (via Node 16.10):

> new Intl.DateTimeFormat('en', {dateStyle:'short'}).resolvedOptions().year
undefined

JSC (as of a recent WebKit build):

> new Intl.DateTimeFormat('en', {dateStyle:'short'}).resolvedOptions().year
"2-digit"

It looks like JSC is not correctly implementing step 5.d of Intl.DateTimeFormat.prototype.resolvedOptions, which is causing a problem with the polyfill, but I believe this remaining test is correct as-is.

@rwaldron
Copy link
Contributor

@ptomato thank you—that satisfies my review! Marking R+, with the understanding that we have a lot of work to do ahead of us, and getting this in sooner is the pragmatic thing to do 👍

@rwaldron
Copy link
Contributor

The CI failure is unrelated to these changes.

@ptomato
Copy link
Contributor Author

ptomato commented Sep 30, 2021

@rwaldron Thanks for the quick review, the help to get this in quickly is much appreciated. I think the next steps for me and @Ms2ger will be to continue to port coverage over from the proposal's legacy test suite (files such as https://github.com/tc39/proposal-temporal/blob/main/polyfill/test/instant.mjs) and then continue with the checklist in #3002.

If you have questions later about any of these tests, or find something that you wish you'd caught in the review, let me know and I will try to help. I realize this form of PR wasn't ideal for you.

I've reported the JSC bug as https://bugs.webkit.org/show_bug.cgi?id=231041

@jugglinmike jugglinmike merged commit 56e537b into tc39:main Oct 1, 2021
@ptomato ptomato deleted the temporal-tests branch October 1, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants