-
Notifications
You must be signed in to change notification settings - Fork 461
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
Temporal tests for "stop coercing non-string primitives to strings" (tc39/proposal-temporal#2574) #3847
Conversation
c600fec
to
a39a423
Compare
a39a423
to
44003e9
Compare
44003e9
to
1eac172
Compare
The PR for these tests, tc39/proposal-temporal#2574, was approved in the July 2023 TC39 meeting, so taking this PR out of draft state. |
1eac172
to
3e95111
Compare
I just rebased to main. Ready for review! |
Edits Temporal tests to account for changes in tc39/proposal-temporal#2574. This PR stops coercing non-string primitive inputs to strings in Temporal methods, to avoid cases where numbers are coerced to syntactically valid but often unexpected string results.
3e95111
to
b213636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed a representative sample of the repetitive tests; obviously not all 300+ of them. I also reviewed the changes to staging. Thanks for the comprehensive work, @justingrant!
I have a minor thing I'd like to change about how the tests are now organized, I think the current division into files no longer makes sense after this change. But that's a "me problem", so I'll open an issue for it and probably make that change myself in a few weeks, as I don't see any reason to hold up this PR! I'd like to ensure that JS engines have access to the newly-correct tests as soon as possible.
(Details: I'd fold the calendar-number
tests into calendar-wrong-type
since there is no longer any special behaviour of numbers to be tested; and I'd split the string tests out of calendar-wrong-type
into something like calendar-invalid-iso-string
because that now throws a different error. But still, I'll fix this myself later.)
Test changes to support tc39/proposal-temporal#2574.