Skip to content
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

WIP: initial attempt to run 'esm' against tc39/test262 #550

Merged
merged 1 commit into from Aug 31, 2018

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Aug 6, 2018

work in progress

notes and questions coming soon

just want to see some runs first 😋

update:

alrighty, cool. looking pretty good so far!

btw, the folder structure etc. where test262 is being cloned into and modified to etc. should probably change. also the way the tests are run (right now as part of the whole test suite). we can separate it out and make it a separate run, or a combination thereof...

couple things I'm noticing which are still to do:

  • skiplist not working on Windows
  • activate other folders, e.g. import, export
  • fix globbing (missed subfolders)
  • make test262 pass on each platform first (use skiplist), then go backwards and unblock
  • add --harmony flag for private class property on supported platforms only (node v10+ probably)
    - [ ] use test262 frontmatter features class-fields-private
  • should we recognize the frontmatter module-flag?
    • investigate: there might be other tests in other folders with that flag
  • investigate node v6.2 failures: probably unsupported features [yep, works on v6.14.3 LTS latest]
  • add with esm options: { cjs:false }
  • investigate: the chakracore run
  • implement flag for ignoring (all or portions) of the skiplist
  • fetch dynamic import test branch from PR
  • run against --experimental-modules

I'll add more later ...

test/tests.js Outdated
@@ -18,6 +18,7 @@ import "./main-hook-tests.mjs"
import "./require-hook-tests.js"
import "./repl-hook-tests.mjs"
import "./scenario-tests.mjs"
import "./_external/test262/index.js"

Copy link
Member

Choose a reason for hiding this comment

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

We call the external folder in src the vendor folder. We could call it that in test too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@@ -0,0 +1,101 @@
# export * from ns
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this list to skiplist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@dnalborczyk

This comment has been minimized.

@jdalton

This comment has been minimized.

@dnalborczyk dnalborczyk force-pushed the test262 branch 2 times, most recently from d904ebb to 025ece4 Compare August 6, 2018 19:43
@dnalborczyk

This comment has been minimized.

@dnalborczyk

This comment has been minimized.

@jdalton

This comment has been minimized.

@dnalborczyk

This comment has been minimized.

@@ -135,3 +135,6 @@ test/language/module-code/namespace/internals/own-property-keys-sort.js

# NEEDS INVESTIGATION!
test/language/module-code/namespace/internals/set.js

# NEEDS INVESTIGATION! -> only fails on node v6.14.3
test/language/module-code/namespace/internals/object-keys-binding-uninit.js
Copy link
Member

Choose a reason for hiding this comment

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

This is just a V8 tweak. In older Node they didn't error. In newer ones they do. We support erroring whenever Node does so this can be gated to whatever Node it's supported in. Related to https://bugs.chromium.org/p/v8/issues/detail?id=7780.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok. it seems to be on the v6 branch only I believe. I'll implement a condition for allowed versions or something like that (e.g. node6, harmony, etc.)

.travis.yml Outdated
@@ -7,7 +7,7 @@ os:
node_js:
- 10
- 8
- 6.2.0
- 6
Copy link
Member

Choose a reason for hiding this comment

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

we'll want to keep that low 6.2.0 is the lowest I can run the full suite at without random seg faults (due to issues in earlier 6.0 releases). It's fine as Node official support is only whatever the latest 6 is but for nice coverage I like to test as far back as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that was just temporary. did you see my question regarding that toppic --> #550 (comment)

I'll extend the skiplist to be conditional. not: all or nothing

test/language/module-code/namespace/internals/get-str-update.js

# NEEDS INVESTIGATION!
test/language/module-code/namespace/internals/own-property-keys-sort.js
Copy link
Member

Choose a reason for hiding this comment

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

We assign namespace keys in sort order so I'd be interested in that fail. 👆

test/language/module-code/namespace/internals/own-property-keys-sort.js

# NEEDS INVESTIGATION!
test/language/module-code/namespace/internals/set.js
Copy link
Member

Choose a reason for hiding this comment

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

We throw here too but be careful as by default esm enables CJS interop with mutable namespace objects (which don't throw). This is configurable though cjs.mutableNamespace.

@@ -99,3 +105,33 @@ test/language/module-code/instn-star-props-nrml.js

# NEEDS INVESTIGATION!
test/language/module-code/parse-err-reference.js

Copy link
Member

Choose a reason for hiding this comment

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

Many of these fails look to be related to having cjs.mutableNamespace enabled 👇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. ok. that's good tho! what's the plan forward to make them pass? is that with mjs disabled as well? I would assume mjs and use module work the same (or similar)?

Copy link
Member

Choose a reason for hiding this comment

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

to avoid .mjs you could just include .esmrc or .esmrc.json in the test files folder or parent folder with specific options disabled.

@dnalborczyk

This comment has been minimized.

@jdalton
Copy link
Member

jdalton commented Aug 31, 2018

Big Big thanks @dnalborczyk!

I'm going to merge this in and then perform some cleanup on it.

@jdalton jdalton merged commit c6b082c into standard-things:master Aug 31, 2018
@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Sep 1, 2018

you're welcome! thank you John! @jdalton

yeah, it probably needs some cleanup.

sorry for being a little inactive lately, just started a new job, and I'm knee deep occupied with onboarding and diving into their codebase. the company is using AWS Lambda, with serverless framework, and needless to say, I tried to bring in esm 😃 and it works great, although only in bridge mode (I believe AWS Lambda currently doesn't support the --require flag as far as I can tell). I noticed something regarding the cache behavior, but I think I better open a new issue for that. If I don't fall asleep I'll also try to open the other issues from some time ago.

you might notice that I'm creating a js folder as well as an mjs folder. I think I had to do some additional "manual" steps (commenting/uncomenting) in order to have it running locally, as a run against node --experimental modules, as well as against esm strict mode to compare the numbers and behavior.

if you want you can ignore that for now, and I'll add it later on again. I wanted to ask you some time ago how you wanted the tests to run ideally once merged in.

Let me know if you have any questions.

edit: everything in the skiplist is being ignored (skipped), except if it contains an additional @-flag, then it'll be only skipped if the condition applies, but runs otherwise.

@dnalborczyk dnalborczyk deleted the test262 branch January 15, 2019 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants