js-temporal / temporal-polyfill

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

Port commits from proposal-temporal 2023-04-16 #244

Closed justingrant closed 1 year ago

justingrant commented 1 year ago

Big batch of migrated commits! I think this is about 40% of the remaining commits, although the huge ones like the string slot changes are still ahead.

Thanks again @12wrigja for the great tools to make this easier.

justingrant commented 1 year ago

Yes, I think that every observable commit that doesn't also change the expected failures file is probably a sign of a possible test gap.

Maybe it's possible in some cases that the tests were deterred? @ptomato may know.

I'll open an issue in the proposal repo to investigate this further.

ptomato commented 1 year ago

Here are the ones where that seems to be the case:

All except the last one in the list have to do with observable order of operations. My guess is that each of those individual commits were necessary but not sufficient to make one of the test262 order-of-operations.js tests pass, so that's why no tests were removed from the expected failures list in each of those commits.

If you can identify the normative PR in the proposal-temporal repo that corresponds to a commit in this repo, I have been fairly consistently linking the test262 PRs that correspond to them (and if I forgot, there's likely a commit in the PR labeled "Update test262" so you can see which test262 commit it points to)

12wrigja commented 1 year ago

@justingrant I wonder if something that might be useful would be a "test262 results" comparison system built into the rebase tool where we could gather up the expected-failures lists from both versions and compare them to see whether there are tests that should have been re-enabled (or disabled) but didn't for some reason? It would probably mean having to maintain another expected-failures-like list of test cases we know should be different from upstream "for reasons", but might make rebasing easier overall?

justingrant commented 1 year ago

I wonder if something that might be useful would be a "test262 results" comparison system built into the rebase tool where we could gather up the expected-failures lists from both versions and compare them to see whether there are tests that should have been re-enabled (or disabled) but didn't for some reason? It would probably mean having to maintain another expected-failures-like list of test cases we know should be different from upstream "for reasons", but might make rebasing easier overall?

The upstream repo has essentially no expected failures, except weird cases like V8 bugs and for those this repo should fail too.

Also, the tests are somewhat out of sync because upstream the Test262 commit is usually the last one in the PR, but here I've been removing tests from the expected failures file for each commit that needs them.

So I'm not sure how what you describe will be helpful. But maybe I'm missing something?

FWIW, dealing with Test262 using the test runner option you built has been a huge help. I just run every Test262 with that option set. So Test262 is essentially a non-issue for the porting workflow... adds zero extra work for me, because my workflow is this:

  1. Open two terminal tabs, and set up the alias in Tab 1 and start the rebase there. Note that I don't run tests using --exec=./test.sh because that wastes time given that I don't commit until all tests are passing. Below is what I do for each step.
  2. Resolve merge conflicts in editor
  3. Make ecmascript.mjs => ecmascript.ts changes in the editor. This is, by far, the most time consuming (and error-prone) part. If we could turn these changes into merge conflicts, then that would be a massive time savings!
  4. Tab 2: Run npm run prune && npm test && npm run test262 -- --update-expected-failure-files
  5. Tab 1: While tests are running, do git add in terminal tab 2 for all changed files (and the expected failure file if needed). If tests fail then I can git rm to fix the problems.
  6. Tab 2: If tests pass, then git rm /polyfill/lib/ecmascript.mjs
  7. Tab 1: trt continue, GOTO 1

After doing this 50+ times I'm getting pretty fast at it. If we can fix the ecmascript thing to be merge conflicts then I could probably 2x+ my pace.