Skip to content

Conversation

@catamorphism
Copy link
Contributor

And move functionality into existing tests.

@catamorphism catamorphism requested review from a team as code owners January 7, 2026 01:47

for (const [arg, description] of primitiveTests) {
assert.throws(
typeof arg === 'string' ? RangeError : TypeError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the empty string in test/built-ins/Temporal/Duration/from/argument-string-invalid.js (this file is called argument-non-string.js!), and then I don't think there's a reason to have two separate arrays and loops.


for (options of [undefined, { overflow: 'constrain' }, { overflow: 'reject' }]) {
assert.throws(
typeof arg === 'string' || (typeof arg === 'object' && arg !== null) || typeof arg === 'function'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is where that came from. Arguably this file shouldn't test for "" (covered in test/built-ins/Temporal/Instant/from/argument-string-invalid.js) or objects at all.


function testRoundtrip(year) {
const dateFromYearMonth = Temporal.PlainDate.from({ year, month: 1, day: 1 }, options);
for ([year, month, monthCode, day] of testData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for ([year, month, monthCode, day] of testData) {
for (const [year, month, monthCode, day] of testData) {

TemporalHelpers.assertPlainDate(
Temporal.PlainDate.from({year: 2021, month: 12, day: 500}),
2021, 12, "M12", 31,
"year/month/day with month and day need to be constrained");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind seeing this test rewritten with a loop either, but won't insist on it here. (But test/built-ins/Temporal/PlainDate/from/with-year-monthCode-day-need-constrain.js should be consistent in either case.)


function testRoundtrip(year) {
const dateFromYearMonth = Temporal.PlainDateTime.from({ year, month: 1, day: 1, hour: 12, minute: 34, second: 56, millisecond: 987, microsecond: 654, nanosecond: 321 }, options);
for ([year, month, monthCode, day] of testData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for ([year, month, monthCode, day] of testData) {
for (const [year, month, monthCode, day] of testData) {


function testRoundtrip(year) {
const dateFromYearMonth = Temporal.ZonedDateTime.from({ year, month: 1, day: 1, hour: 12, minute: 34, second: 56, millisecond: 987, microsecond: 654, nanosecond: 321, timeZone: "UTC" }, options);
for ([year, month, monthCode, day] of testData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for ([year, month, monthCode, day] of testData) {
for (const [year, month, monthCode, day] of testData) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants