Temporal: Update PlainMonthDay-related tests by gibson042 · Pull Request #3899 · tc39/test262

gibson042 · 2023-08-21T21:23:56Z

As of tc39/proposal-temporal#2500 , year is always optional for the ISO 8601 calendar.

gibson042

Apologies for not separating this into meaningful commits; I have added PR comments to categorize.

gibson042 · 2023-08-22T14:26:03Z

+assert.throws(TypeError, () => cal.monthDayFromFields({ month: 12 }), "day is required with month");
 assert.throws(TypeError, () => cal.monthDayFromFields({ monthCode: "M12" }), "day is required with monthCode");
 assert.throws(TypeError, () => cal.monthDayFromFields({ year: 2021, month: 12 }), "day is required with year and month");
-assert.throws(TypeError, () => cal.monthDayFromFields({ month: 1, day: 17 }), "year is required if month is present");


This assertion is no longer appropriate because monthDayFromFields({ month, day }) is acceptable in the ISO 8601 calendar.

(case 2.ii)

gibson042 · 2023-08-22T14:27:12Z

+options.forEach((opt) => {
+  const optionsDesc = opt && JSON.stringify(opt);
  result = cal.monthDayFromFields({ year: 2021, month: 7, day: 3 }, opt);
-  TemporalHelpers.assertPlainMonthDay(result, "M07", 3, "month 7, day 3, with year");
+  TemporalHelpers.assertPlainMonthDay(result, "M07", 3, `month 7, day 3, with year, options = ${optionsDesc}`);
  result = cal.monthDayFromFields({ year: 2021, month: 12, day: 31 }, opt);
-  TemporalHelpers.assertPlainMonthDay(result, "M12", 31, "month 12, day 31, with year");
+  TemporalHelpers.assertPlainMonthDay(result, "M12", 31, `month 12, day 31, with year, options = ${optionsDesc}`);
  result = cal.monthDayFromFields({ monthCode: "M07", day: 3 }, opt);
-  TemporalHelpers.assertPlainMonthDay(result, "M07", 3, "monthCode M07, day 3");
+  TemporalHelpers.assertPlainMonthDay(result, "M07", 3, `monthCode M07, day 3, options = ${optionsDesc}`);
  result = cal.monthDayFromFields({ monthCode: "M12", day: 31 }, opt);
-  TemporalHelpers.assertPlainMonthDay(result, "M12", 31, "monthCode M12, day 31");
+  TemporalHelpers.assertPlainMonthDay(result, "M12", 31, `monthCode M12, day 31, options = ${optionsDesc}`);
+});


This DRYs out the testing and increases coverage but does not change expectations.

gibson042 · 2023-08-22T14:30:42Z

+    return 1;
  },
 };
-assert.throws(TypeError, () => instance.monthDayFromFields(monthWithoutYear), "year/month should be checked after fetching but before resolving the month code");


This assertion is no longer appropriate because monthDayFromFields({ month, day }) is acceptable in the ISO 8601 calendar, but wasn't actually material to the test so has been replaced with assertions against a full complement of property access spies that also subsume { day: 1 }.

gibson042 · 2023-08-22T14:32:55Z

-assert.throws(
-  RangeError,
-  () => cal.monthDayFromFields({ year: 2021, month: -99999, day: 1 }, opt),
-  "negative month -99999 is out of range even with overflow constrain"
-)
-assert.throws(
-  RangeError,
-  () => cal.monthDayFromFields({ year: 2021, month: -1, day: 1 }, opt),
-  "negative month -1 is out of range even with overflow constrain"
-)
-assert.throws(
-  RangeError,
-  () => cal.monthDayFromFields({ year: 2021, month: 0, day: 1 }, opt),
-  "month zero is out of range even with overflow constrain"
-)


These have been moved to the first forEach.

gibson042 · 2023-08-22T14:37:11Z

+  result = cal.monthDayFromFields({ month, day: daysInMonth });
+  TemporalHelpers.assertPlainMonthDay(result, monthCode, daysInMonth, `month ${month}, day ${daysInMonth}`);


This monthDayFromFields would previously have been expected to throw.

Note to self: case 2.ii from tc39/proposal-temporal#2664

gibson042 · 2023-08-22T14:49:01Z

+  assert.throws(RangeError,
+    () => cal.monthDayFromFields({ month, day }, { overflow: "reject" }),
+    `Day ${day} is out of range for month ${month} with overflow: reject`);


This monthDayFromFields would previously have been expected to throw, but for a different reason.

gibson042 · 2023-08-22T14:50:12Z

+    `Day ${day} is out of range for monthCode ${monthCode} with overflow: reject`);
+});
+
+[ ["month", 2], ["monthCode", "M02"] ].forEach(([ name, value ]) => {


This is new coverage for year sensitivity (but use of monthCode would have been expected to ignore year before tc39/proposal-temporal#2497 ).

gibson042 · 2023-08-22T14:52:07Z

-assert.throws(TypeError, () => Temporal.PlainMonthDay.from({ month: 11, day: 18, calendar: "iso8601" }), "month, day with calendar");
+
+if (TemporalHelpers.nonDefaultCalendarId) {
+  assert.throws(TypeError, () => Temporal.PlainMonthDay.from({ month: 11, day: 18, calendar: TemporalHelpers.nonDefaultCalendarId }), "month, day with non-iso8601 calendar");
+}


The expected failure is now dependent upon use of a non–ISO-8601 calendar.

I'm not sure that this is a change we want to make. See tc39/proposal-temporal#2663 (comment)

gibson042 · 2023-08-22T14:54:34Z

+TemporalHelpers.assertPlainMonthDay(md.with({ month: 12 }),
+  "M12", 15, "with({month})");
+
 TemporalHelpers.assertPlainMonthDay(md.with({ monthCode: "M12" }),
  "M12", 15, "with({monthCode})");

-assert.throws(TypeError, () => md.with({ month: 12 }), "with({month})");


This with would previously have been expected to throw, but no longer does in the ISO 8601 calendar.

ptomato

Thanks for the explanatory comments, they really made this easier.
These look good, I noticed one typo in an assertion message and I'd like to move the non-ISO-calendar-dependent tests into test/intl402/.

ptomato · 2023-09-18T21:51:23Z

+  TemporalHelpers.assertPlainMonthDay(result, monthCode, daysInMonth, `month ${month}, day ${daysInMonth}`);
+
+  result = cal.monthDayFromFields({ monthCode, day: daysInMonth });
+  TemporalHelpers.assertPlainMonthDay(result, monthCode, daysInMonth, `monthCode ${monthCode}, day ${daysInMonth}`);


ptomato · 2023-09-18T22:15:20Z

So, I feel pretty strongly about not having the tests for nonDefaultCalendarID execute conditionally, because we generally don't like conditionally executing tests, but also particularly because it's not what we've done in the rest of the Temporal test coverage.

As a way forward, could I propose the following? Let's move the nonDefaultCalendarID stuff to the test/intl402/ folders for now and just pick a calendar ID that's likely to be implemented, like "hebrew" or "gregory". Then separately work on migrating all of the tests that require a non-ISO8601 calendar but aren't dependent on it being any particular calendar, to use a new flag in the front matter. Straw proposal something like flags: [non-iso-calendar]. Runners that support this flag should provide a $262.nonISOCalendarID property which is a string calendar ID, and otherwise skip the test.

Also I'd suggest making a couple of files (one in test/built-ins/ and one in test/intl402/) that explicitly test each case that we listed in tc39/proposal-temporal#2664. It's mostly points 1, 2, and 3 that we have coverage for, 4 and 5 not so much (unless it already exists elsewhere and I don't see it.)

ptomato · 2023-09-18T22:18:30Z

Forgot to add, I realize that's a lot of refactoring to ask, so if that'll be a prohibitive time sink, let me know and I can do it.

gibson042 · 2023-09-18T23:23:33Z

So, I feel pretty strongly about not having the tests for nonDefaultCalendarID execute conditionally, because we generally don't like conditionally executing tests, but also particularly because it's not what we've done in the rest of the Temporal test coverage.

Allright, I don't object.

As a way forward, could I propose the following? Let's move the nonDefaultCalendarID stuff to the test/intl402/ folders for now and just pick a calendar ID that's likely to be implemented, like "hebrew" or "gregory". Then separately work on migrating all of the tests that require a non-ISO8601 calendar but aren't dependent on it being any particular calendar, to use a new flag in the front matter. Straw proposal something like flags: [non-iso-calendar]. Runners that support this flag should provide a $262.nonISOCalendarID property which is a string calendar ID, and otherwise skip the test.

That's probably overkill, but I support the general concept.

Also I'd suggest making a couple of files (one in test/built-ins/ and one in test/intl402/) that explicitly test each case that we listed in tc39/proposal-temporal#2664. It's mostly points 1, 2, and 3 that we have coverage for, 4 and 5 not so much (unless it already exists elsewhere and I don't see it.)

Hmm, I'm not sure how to structure that but I also support it. I think I am going to avail myself of your offer to refactor.

ptomato · 2023-09-19T01:11:45Z

I think I am going to avail myself of your offer to refactor.

All good, I'll do that tomorrow.

ptomato · 2023-09-19T18:15:47Z

@@ -0,0 +1,31 @@
+// Copyright (C) 2023 Igalia, S.L. All rights reserved.


Added to verify that a custom calendar can do its own verification.

ptomato · 2023-09-19T18:17:17Z

-description: Throw a RangeError if only one of era/eraYear fields is present
+description: Throw a TypeError if only one of era/eraYear fields is present
 features: [Temporal]
 ---*/

 const base = { year: 2000, month: 5, day: 2, era: 'ce' };
 const instance = new Temporal.Calendar('gregory');
-assert.throws(RangeError, () => {
+assert.throws(TypeError, () => {
  instance.dateFromFields({ ...base });
 });

 const base2 = { year: 2000, month: 5, day: 2, eraYear: 1 };
-assert.throws(RangeError, () => {
+assert.throws(TypeError, () => {


This (and the similar change in yearMonthFromFields) is an additional pre-existing bug that I found while working on this. CalendarResolveFields says "The operation throws a TypeError exception if the properties of fields are internally inconsistent within the calendar or insufficient to identify a unique instance of type in the calendar," but we were expecting a RangeError here.

ptomato · 2023-09-19T18:17:58Z

@@ -1,19 +0,0 @@
-// Copyright (C) 2023 Igalia, S.L. All rights reserved.


Renamed to fields-underspecified.js because I added more cases that were not specifically about era/eraYear.

ptomato · 2023-09-19T18:23:25Z

@gibson042 Have a look at the new commits and let me know what you think!

While working on this I uncovered an additional bug in the tests, see comments for more information.

ptomato · 2023-10-02T21:42:56Z

@gibson042 I'm happy with the state of this now, but could you take a quick look at what I added on top of your work? Feel free to click the merge button if you're OK with it.

gibson042

@ptomato LGTM, thanks for your help (and your patience)!

As of tc39/proposal-temporal#2500 , year is always optional for the ISO 8601 calendar.

We'll do this for now, then separately work on migrating all of the tests that require a non-ISO8601 calendar but aren't dependent on it being any particular calendar.

Built-in non-ISO calendars require either monthCode/day, or month/day plus some form of year specification. This adds test coverage for each of the categories listed in tc39/proposal-temporal#2664, of which some must currently reside in the test/intl402/ folders.

These tests were incorrect, in checking for a RangeError when only one of the era/eraYear fields were given. From CalendarResolveFields: "The operation throws a *TypeError* exception if the properties of _fields_ are internally inconsistent within the calendar or insufficient to identify a unique instance of _type_ in the calendar."

gibson042 requested a review from ptomato August 21, 2023 21:23

gibson042 requested a review from a team as a code owner August 21, 2023 21:23

gibson042 mentioned this pull request Aug 21, 2023

Polyfill: Update per #2500 tc39/proposal-temporal#2663

Merged

gibson042 commented Aug 22, 2023

View reviewed changes

ptomato approved these changes Aug 22, 2023

View reviewed changes

ptomato reviewed Sep 18, 2023

View reviewed changes

Comment thread test/built-ins/Temporal/Calendar/prototype/monthDayFromFields/missing-properties.js Outdated

ptomato force-pushed the 2023-08-iso8601-monthday branch from 5772f76 to 6f472ff Compare September 19, 2023 18:10

ptomato reviewed Sep 19, 2023

View reviewed changes

ptomato force-pushed the 2023-08-iso8601-monthday branch from 61001f0 to 27721b4 Compare September 19, 2023 18:21

ptomato mentioned this pull request Sep 19, 2023

Final normative spec text plan tc39/proposal-temporal#2628

Closed

91 tasks

ptomato force-pushed the 2023-08-iso8601-monthday branch from 27721b4 to 96fedf9 Compare September 28, 2023 15:07

ptomato added the awaiting author label Oct 2, 2023

gibson042 commented Oct 26, 2023

View reviewed changes

gibson042 and others added 5 commits October 26, 2023 12:34

Temporal: Update PlainMonthDay-related tests

c42f75c

As of tc39/proposal-temporal#2500 , year is always optional for the ISO 8601 calendar.

Temporal: Move non-ISO calendar tests to intl402/

ca2c30e

We'll do this for now, then separately work on migrating all of the tests that require a non-ISO8601 calendar but aren't dependent on it being any particular calendar.

Temporal: Use propertyBagObserver in monthDayFromFields test

49b08a8

gibson042 force-pushed the 2023-08-iso8601-monthday branch from 96fedf9 to 49b08a8 Compare October 26, 2023 16:35

gibson042 enabled auto-merge (rebase) October 26, 2023 16:35

gibson042 merged commit 892a5dc into tc39:main Oct 26, 2023

		result = cal.monthDayFromFields({ month, day: daysInMonth });
		TemporalHelpers.assertPlainMonthDay(result, monthCode, daysInMonth, `month ${month}, day ${daysInMonth}`);

		@@ -0,0 +1,31 @@
		// Copyright (C) 2023 Igalia, S.L. All rights reserved.

		@@ -1,19 +0,0 @@
		// Copyright (C) 2023 Igalia, S.L. All rights reserved.

Conversation

gibson042 commented Aug 21, 2023

Uh oh!

gibson042 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ptomato commented Sep 18, 2023

Uh oh!

ptomato commented Sep 18, 2023

Uh oh!

gibson042 commented Sep 18, 2023

Uh oh!

ptomato commented Sep 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato commented Sep 19, 2023

Uh oh!

ptomato commented Oct 2, 2023

Uh oh!

gibson042 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ptomato Sep 18, 2023 •

edited

Loading