-
Notifications
You must be signed in to change notification settings - Fork 461
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
Testing the Temporal proposal #3002
Comments
Thanks for getting this started! FWIW, I've been finding it easier to write tests by topic, rather than by API entrypoint — for example, here are a series of largely identical tests for everything that uses the GetOption abstract operation: https://github.com/tc39/proposal-temporal/pull/1531/files One thing I'm planning to do is convert the Mocha-style tests of the research polyfill in that repo to test262-style tests, and delete the old-style tests as I go. So anyone picking one of these items can use those old-style tests as a starting point. |
Thanks, @jugglinmike! I think that for the moment it would be easiest to keep developing the tests in the proposal repo, so we can run them against the polyfill as a sanity check. Does that make sense to you? |
Hopefully the polyfill will be finished and deprecated soon tho, so the advantage to keeping the tests there shouldn’t last long? |
@Ms2ger I've had luck using Node.js and the test262-harness project to run Test262 against the polyfill: $ test262-harness \
--hostType node \
--hostPath node \
--prelude inject-temporal.js \
'test/built-ins/Temporal/**/*' Where const { Temporal } = require('./node_modules/proposal-temporal'); I'm using the version published to npm, but it ought to work for the proposal's repository, too. Does that help at all? |
@jugglinmike For one thing, we're planning to deprecate that NPM module... An advantage to keeping it in the proposal repository is that we have code coverage metrics for the test262 tests there, and it doesn't require synchronization between git repositories. (And I can't remember the details off the top of my head, but using |
I left one comment in #3049 but it could simplify things in all of these pull requests:
|
@jugglinmike I noticed that all the getters are missing from this list - here's a change to the code snipped you posted to include them in the list: -Array.from(document.querySelectorAll('[id^="sec-temporal."]'))
+Array.from(document.querySelectorAll('[id^="sec-temporal."], [id^="sec-get-temporal."]'))
.map((el) => {
return el
.querySelector('h1')
.innerText
- .replace(/^[0-9\. ]*([^ ([]+)(\[[^\]]+])?.*/, '- [ ] $1$2');
+ .replace(/^[0-9\. ]*(?:get )?([^ ([]+)(\[[^\]]+])?.*/, '- [ ] $1$2');
})
.join('\n'); You also might want to update |
It's clear that there's no full coverage yet, but it's not clear to me how it's helpful to do the work to review the coverage outside this repo. On the other hand, having them in this repo would be very helpful for the people who are implementing the proposal as we speak. |
This is actually really compelling. Which engines are currently implementing? |
Hi @rwaldron! I think my concern is actually the opposite — I don't want two sets of tests, only one! We would like to delete the tests from tc39/proposal-temporal as soon as they are present in tc39/test262, and pretty much have a setup prepared just like the one you outlined. I'm just concerned about the two sets of tests getting out of sync during review, if the PR would (understandably, because large) take a long time to merge, because that would cause a lot of extra work for me. That's why I asked if it could be merged swiftly and stylistic points cleaned up later. I get your point about coverage, but maybe that could be mitigated using the checklist in the OP of this issue? As we check off each item in the checklist, we could consider the coverage for that item audited. The difference would be that someone working on a checklist item would start from incomplete coverage instead of from zero, which has disadvantages as well as advantages. The proposal is currently being implemented by V8, SpiderMonkey, and JSC. One other implementation that I'm aware of is in SerenityOS's engine. |
Hey there, @ptomato. Let's do it! @rwaldron and I talked this through today. Although we'd prefer to be more familiar with the test material before merging it, we think the opportunity to assist in-progress implementations, coupled with the correctness (if not completeness) of the tests merged so far, warrant expedition. |
Thanks, that's great news! I am on vacation tomorrow, so will make this pull request as soon as possible on Monday. If you would like to plan a Matrix chat session or video call next week to go through the test material so I can help you get that familiarity, I would certainly be available for that. |
Thanks, Philip, that's very kind! Regarding coordination: I personally won't be available for reviews until Thursday of next week (that is, September 30). @rwaldron should be around on Monday, but we'll collectively have more bandwidth at the end of the week. You can send a pull request whenever you like, of course--I just know you're interested in limiting the review period. |
OK, thanks! In that case I won't prioritize the pull request today, but I'll make sure to send it by Wednesday. Does that sound good to you? |
This copies over the tests that previously existed in the tc39/proposal-temporal repository. For context, see thread starting at: tc39#3002 (comment) In service of tc39#3002
This copies over the tests that previously existed in the tc39/proposal-temporal repository. For context, see thread starting at: tc39#3002 (comment) In service of tc39#3002
Works for me! |
This copies over the tests that previously existed in the tc39/proposal-temporal repository. For context, see thread starting at: tc39#3002 (comment) In service of tc39#3002
This copies over the tests that previously existed in the tc39/proposal-temporal repository. For context, see thread starting at: #3002 (comment) In service of #3002
Are the checkboxes in the OP of this thread still accurate? Or have some additional methods been fully covered by Temporal's 3000+ tests? |
I don't think anyone has reviewed the coverage we gained by landing the polyfill tests - I'll see if someone on our side can do it |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Will there be tests for each different calendar? Calendars are not symmetric like timezones and it's easy to mistake in only one of them. |
We are currently focusing on the ISO calendar, so deep testing of the actual calendar operations is likely a while off. I imagine we would take tests if someone else had time to write them, though. |
I'm wondering what the policy on these should be, as well. Tests for time zones can be difficult because time zones can be adjusted by civil authorities. If that happened, tests could start failing whereas that would not indicate any standards noncompliance, just outdated time zone data. Calendars are somewhat less subject to change, but still, I do know of a few cases that could cause tests to be invalidated. The Hijri calendar may get correction days depending on astronomical observations (see heading "Difficult to Predict" of this document) and any calendar that uses eras may decide to create a new era in the future. |
I feel lacking tests for time zones is fine since time zones are pure data, and implementation can be auto generated from IANA database. but calendars need some code as well, so at least there should be some tests for different classes of calendars. But with this logic, something like era doesn't need test since it is data-ish. Anyway this is not
I'm volunteer to write some tests if policy becomes that we need tests. Also I need specification of calendars for writing tests which I didn't find them in ecma-402. Where does it exists? |
@HKalbasi Sorry, I missed your comment back in February. There isn't any specification of calendar arithmetic because it's expected that ECMA-402 delegates it to a library like ICU4C or ICU4X as they do with locale-specific formatting. So, if you write tests for calendar arithmetic I'd recommend doing it on a best-effort / representative-sample basis. |
There are a lot of error throwing behavior need to be tested in for example step 6-a of and step 6-a of also Step 5-a of |
The Temporal proposal reached Stage 3 on 2021-03-10, and like all ECMA262 proposals, it needs coverage in Test262 to advance to Stage 4. In terms of normative text, it is easily the largest proposal we've seen. Testing it will definitely require significant time, effort, and coordination (even with the proposal's existing tests as a starting point). We'd like to document progress publicly for transparency and to encourage participation.
To get the ball rolling, I'm suggesting that we track progress on an per-API basis using this GitHub.com issue. Compared to alternatives like collaborative document editing services and an in-repository text file, this seems like it balances ease of hosting, maintainability, and proximity to the tests. This approach worked well for ES6's well-known Symbols--the most similar (though much smaller) effort that comes to mind.
Working mode
Anyone can comment here when they'd like to begin writing tests for a specific API. A maintainer will update the list to reflect the ongoing and completed work. When you submit a pull request, please mention this issue (just write "gh-3002") so GitHub displays the status of your work in this thread.
Tests for multiple APIs can be submitted in a single pull request, but patches should be small enough for another contributor to review them in a single session. For instance, it probably makes sense to submit tests for all of the
@@toStringTag
methods in one patch. There's some subjectivity here, but when in doubt, we should err on the side of smaller patches (and remain open to reviewers' requests to break up pull requests).Status
(generated via the following script)
I executed that in the browser's Developer Console on the proposal's specification text.
Notes about specific things that should have test coverage:
roundingMode: 'halfExpand'
insince
for negative numbers with 0.5 remainders proposal-temporal#1111)diffUntil = earlier.until(later, { largestUnit: 'days' })
anddiffSince = later.since(earlier, { largestUnit: 'days' })
, we need to test cases where:diffUntil
not equal todiffSince
earlier.add(diffUntil)
equalslater
later.subtract(diffUntil)
does not equalearlier
later.subtract(diffSince)
equalsearlier
earlier.add(diffSince)
does not equallater
gregory
calendar coverage, possibly via codegen, to check it does the same arithmetic asiso8601
foryear > 1
(from Add morecalendar: 'gregory'
coverage to Test262 tests and/or demitasse tests proposal-temporal#1776)timeZone.getInstantFor
throw for non-Temporal.Instant valuesPlainMonthDay.prototype.toPlainDate
correctly handle[['year', undefined]]
for input fieldsObject.assign(new Temporal.MonthDay(1, 1), { year: 2000 }))
calendar.fields
is always called with the correct list of fieldscalendar.fields
toString
is not called in CalendarEquals, TimeZoneEquals, ConsolidateCalendars if the two calendars are the same object (Some Calendar questions proposal-temporal#1425 point 2)PT99999....99999Y
)(optionName)-undefined.js
tests also test plain function objects11111111
or1111-11-11
) as the calendar name throws, and doesn't default to the ISO calendar (from Missing calendar validation in ToRelativeTemporalObject and ToTemporalZonedDateTime proposal-temporal#2546 (comment))order-of-operations.js
tests) - see AddDurationToOrSubtractDurationFromPlainYearMonth: Move GetOptionsObject closer to the top proposal-temporal#2721The text was updated successfully, but these errors were encountered: