-
Notifications
You must be signed in to change notification settings - Fork 464
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 tests for AsyncContext (Stage 2) #3874
base: main
Are you sure you want to change the base?
Conversation
The tests in Also, |
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.
disclaimer: I may be missing some adjustments to generators in https://tc39.es/proposal-async-context/, but at a glance I wasn't able to find anything yet.
test/built-ins/AsyncContext/context-propagation/generators-yield-inside-run.js
Show resolved
Hide resolved
test/built-ins/AsyncContext/context-propagation/async-generators-yield-inside-run.js
Show resolved
Hide resolved
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've only reviewed the Snapshot cases so far, but a few more cases we could add:
snapshot.run()
usesundefined
this contextsnapshot.run()
restores previous context after abrupt throw
test/built-ins/AsyncContext/Variable/constructor-throwing-name-hasproperty.js
Outdated
Show resolved
Hide resolved
test/built-ins/AsyncContext/context-propagation/CreateIteratorFromClosure.js
Outdated
Show resolved
Hide resolved
test/built-ins/AsyncContext/Variable/prototype/run/restores-context-snapshot-after-callback.js
Outdated
Show resolved
Hide resolved
test/built-ins/AsyncContext/Variable/prototype/run/restores-context-snapshot-after-callback.js
Outdated
Show resolved
Hide resolved
|
||
setAndAssertLength(undefined, 0); | ||
|
||
setAndAssertLength("42", 42); |
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.
Well this wasn't intentional. I was just trying to avoid the length args, and it looked like LengthOfArrayLike
did everything I wanted. Do we want to define this more rigorously?
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 don't really care one way or the other, but Function.prototype.bind
checks whether the length is a number before calling ToIntegerOrInfinity()
, and if it isn't one it uses 0 rather than converting to a number.
Also, now that I look at it in some more detail, I realize that bind
also checks HasOwnProperty
before doing Get
, whereas the wrap
spec only does Get
, so this might affect proxies. When we settle on this (one way or the other), we should also test it.
In any case, spec-wise the ShadowRealm proposal adds a CopyNameAndLength
AO to share the behavior between bind
and ShadowRealm wrapped functions. If we decide to have the same behavior as bind
, it'd be better to just use that if we can.
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've updated the tests to the behavior CopyNameAndLength
has. PTAL.
See https://github.com/tc39/proposal-async-context.