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

Temporal tests for "stop coercing non-string primitives to strings" (tc39/proposal-temporal#2574) #3847

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Jun 9, 2023

Test changes to support tc39/proposal-temporal#2574.

@justingrant justingrant force-pushed the temporal-2574 branch 6 times, most recently from c600fec to a39a423 Compare June 11, 2023 05:12
@justingrant justingrant changed the title DRAFT: Temporal tests for "stop coercing non-string inputs to strings" DRAFT: Temporal tests for "stop coercing non-string primitives to strings" Jun 11, 2023
@justingrant justingrant marked this pull request as ready for review July 16, 2023 03:28
@justingrant justingrant requested review from a team as code owners July 16, 2023 03:28
@justingrant
Copy link
Contributor Author

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.

@justingrant justingrant changed the title DRAFT: Temporal tests for "stop coercing non-string primitives to strings" DRAFT: Temporal tests for "stop coercing non-string primitives to strings" (tc39/proposal-temporal#2574) Jul 16, 2023
@justingrant justingrant changed the title DRAFT: Temporal tests for "stop coercing non-string primitives to strings" (tc39/proposal-temporal#2574) Temporal tests for "stop coercing non-string primitives to strings" (tc39/proposal-temporal#2574) Jul 16, 2023
@justingrant
Copy link
Contributor Author

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.
Copy link
Contributor

@ptomato ptomato left a 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.)

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.

2 participants