-
Notifications
You must be signed in to change notification settings - Fork 475
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
Add syntax tests for import.defer
#4374
Conversation
919e9f8
to
a9a33ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor comments, otherwise looks good.
(I reviewed only the .case
files, and only glanced at the generated tests.)
template: syntax/invalid | ||
info: | | ||
ImportCall : | ||
import . source ( AssignmentExpression[+In, ?Yield] ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import . source ( AssignmentExpression[+In, ?Yield] ) | |
import . defer ( AssignmentExpression[+In, ?Yield, ?Await] ) |
//- setup | ||
// This is still valid in script code, and should not be valid for module code | ||
// https://tc39.github.io/ecma262/#sec-scripts-static-semantics-lexicallydeclarednames | ||
var smoosh; function smoosh() {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realize that we had automatic setup
and teardown
substitution variables in the test generator! (They are documented; I just never noticed.)
Anyway, not having flags: [module]
in the test seems like it's sufficient for ensuring it's executed as script code, so is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this from the import.source
tests (these .case
files are basically a copy-paste of those). It is also use in the import
tests. I'm going to remove it from the import.defer
tests, but probably it should then also be removed from those, since it's indeed useless (unless we want to catch broken test262 runners).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, would you like to make a follow-up PR for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open an issue assigned to myself for now :P
I tried running this on the WebKit prototype implementation and the implementation and tests agreed, so looks good from that perspective. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
These tests are all based on the existing dynamic import syntax tests, just updated to use the new syntax. This PR does not contain any manually written tests -- they are all generated.
5b558ea
to
a9d3eb6
Compare
These tests are all based on the existing dynamic import syntax tests, just updated to use the new syntax.
This PR does not contain any manually written tests -- they are all generated.
Ref #4215