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: Test rejecting ISO strings with subminute offsets #3877

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

justingrant
Copy link
Contributor

This PR verifies that ISO strings with sub-minute offsets cannot be parsed into time zone identifiers. This was a change introduced in the recently-merged tc39/proposal-temporal#2607, but tests for this case were missing from #3862 (the tests for that PR).

I noticed in codecov results on an unrelated PR that this case wasn't being tested, so fixing that mistake now.

@justingrant justingrant requested a review from a team as a code owner July 21, 2023 05:59
@justingrant justingrant changed the title Test rejecting ISO strings with subminute offsets Temporal: Test rejecting ISO strings with subminute offsets Jul 21, 2023
@justingrant justingrant force-pushed the iso-string-timezone-subminute branch 3 times, most recently from 3b3bce5 to 976c865 Compare July 21, 2023 06:47
@justingrant justingrant requested a review from a team as a code owner July 21, 2023 08:55
justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Jul 21, 2023
To make it easier to find actual Test262 gaps, this PR removes
unreachable code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from parsing.
Doing this exposed a test bug that's fixed in the second commit of
tc39/test262#3877.

I added a TODO comment to remove offset component parsing from
ParseISODateTime, because it's unused.
justingrant added a commit to justingrant/proposal-temporal that referenced this pull request Jul 21, 2023
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.
@Ms2ger Ms2ger force-pushed the iso-string-timezone-subminute branch from 2aa65fd to cc70fc0 Compare July 25, 2023 09:27
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.

Thanks!

This commit verifies that ISO strings with sub-minute offsets cannot
be parsed into time zone identifiers. This was a change introduced in
the recently-merged tc39/proposal-temporal#2607, but tests for this case
were missing from tc39#3862 (the tests for that PR).

I noticed in codecov results on an unrelated PR that this case wasn't
being tested, so fixing that mistake now.
There was one remaining Temporal test that was parsing an ISO string
with a sub-minute offset into a Temporal.TimeZone. A polyfill bug was
allowing this test case to pass, even though after
tc39/proposal-temporal#2607 it should have failed.

This commit removes the bad test case.
@ptomato ptomato force-pushed the iso-string-timezone-subminute branch from cc70fc0 to cb24feb Compare August 9, 2023 18:32
@ptomato ptomato merged commit 66f3959 into tc39:main Aug 9, 2023
1 check passed
ptomato pushed a commit to justingrant/proposal-temporal that referenced this pull request Aug 9, 2023
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.
ptomato pushed a commit to tc39/proposal-temporal that referenced this pull request Aug 9, 2023
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Feb 26, 2024
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.

UPSTREAM_COMMIT=212fffcd2df3d5ac6dbcc6abfb628d696c5de146
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Mar 4, 2024
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.

UPSTREAM_COMMIT=212fffcd2df3d5ac6dbcc6abfb628d696c5de146
justingrant added a commit to js-temporal/temporal-polyfill that referenced this pull request Mar 5, 2024
To make it easier to find actual Test262 gaps, this PR removes
unreachable and unused code in ISO parsing.

It also removes offset normalization code from ParseISODateTime,
to better mach the spec that only normalizes downstream from ISO
string parsing. Doing this exposed a test bug that's fixed in the second
commit of tc39/test262#3877.

UPSTREAM_COMMIT=212fffcd2df3d5ac6dbcc6abfb628d696c5de146
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