js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

getoffsetnanosecondsfor tests might be inconsistently passing/failing #213

Open 12wrigja opened 1 year ago

12wrigja commented 1 year ago

While debugging CI failures in #210 , it seems that only some of the test262 cases for toLocaleString/timezone-getoffsetnanosecondsfor-not-callable.js fail, and some pass:

Passing: PlainTime PlainDate PlainDateTime

Failing: PlainYearMonth PlainMonthDay DateTimeFormat/prototype/format DateTimeFormat/prototype/formatToParts DateTimeFormat/prototype/formatRange DateTimeFormat/prototype/formatRangeToParts

I haven't looked at all the testcases but given they are likely all very similarly structured I'm wondering this is indicative of a bug somewhere?

Failing that, it would be nice to improve the documentation to explain why those tests fail on Node<16 more clearly.

justingrant commented 1 year ago

Is the difference that the passing ones are calling toLocaleString with no arguments, while the failing ones are calling with these arguments?

instance.toLocaleString(undefined, { calendar: "iso8601" })

It may be that Node 14 didn't support the calendar option, so it's failing with a RangeError before the uncallable thing is called?

So it's not like these tests run without exceptions in any case. It's only that the ones with a calendar option fail in an unexpected way (RangeError) on Node 14, while the ones without a calendar option fail in an expected way (TypeError).

Anyway, I think it's OK to just put the failing tests into the expected-failures-before-node16.txt and leave the passing tests (the ones that fail with the expected TypeError) out of that file.