Tests for audit of Temporal user code calls, part 2 #3897
Conversation
ptomato
changed the title
ptomato
force-pushed
the
user-code-calls-part-2
branch
from
5874fda to
262274e
Compare
ptomato
force-pushed
the
user-code-calls-part-2
branch
from
262274e to
cf23b46
Compare
ptomato
force-pushed
the
user-code-calls-part-2
branch
3 times, most recently
from
cf23b46 to
a26d564
Compare
|
Looks great to me. Covers all the different entry points for each operation. Really thorough - discussed some feedback directly while working through the commits, and you fixed the spec bug I found in the no-op round operations before I got to comment it on the review :P only extant feedback: For the tests in 'Don't observably iterate array in built-in calendar's fields()', do we have any tests already that cover that an otherwise unmodified |
I made various modifications to the reference polyfill and re-ran the tests, to check this. I think the answer is, we do not have any tests for this directly. (If you modify CalendarFields to skip the iteration when calendar has the correct internal slot and Get(calendar, "fields") is the same value as %Temporal.Calendar.prototype.fields%, then no tests fail.) However, we do have tests that will fail if an implementation makes the mistake that would generally be the cause of this situation, that is, treating an unmodified instance of a built-in calendar the same as a string calendar. |
This shortcut path now exists in all round(), since(), and until() operations. In Instant, PlainDate, PlainDateTime, and PlainTime, the change isn't observable, so no tests could be added. This adds test coverage for - Duration.p.round() - PlainYearMonth.p.since() - PlainYearMonth.p.until() - ZonedDateTime.p.round() - ZonedDateTime.p.since() - ZonedDateTime.p.until() As well as a few cases where we are testing that certain calendar methods get called during a round operation, but previously were doing so with options that now become a no-op and no longer call those calendar methods. In those cases, round to 2 ns, rather than 1 ns.
Adds new tests to order-of-operations.js in Duration.round and Duration.total, to exercise the code path where previous to this normative change, relativeTo would have been converted to PlainDate 3x and 2x, respectively.
Note the monkeypatch of getPossibleInstantsFor in test/built-ins/Temporal/ TimeZone/prototype/getInstantFor/argument-builtin-calendar-no-array- iteration.js. Other than that, all the tests are basically identical.
Ms2ger
force-pushed
the
user-code-calls-part-2
branch
from
a26d564 to
7c87e55
Compare
Continuing on from #3894, tc39/proposal-temporal#2519 is a very large pull request and I'm planning to land it in parts. This PR contains tests for the second part. Each commit here corresponds to the same-named commit in the proposal-temporal normative PR.
Recommend reviewing commit-by-commit. Note, in the commit titled "Temporal: Don't observably iterate array in built-in calendar's fields()" there are a large number of tests that are basically copy-pasted except for one, as noted in the commit message, so double-check that one when reviewing.