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

Add tests for AsyncContext (Stage 2) #3874

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

andreubotella
Copy link
Member

@andreubotella
Copy link
Member Author

The tests in built-ins/AsyncContext/context-propagation/* are really tests that the changes to the HostMakeJobCallback and HostCallJobCallback host hooks are indeed reflected in the built-in APIs that use those hooks. Maybe they should be moved to those APIs' folders.

Also, built-ins/AsyncContext/context-propagation/generators.js currently tests a behavior that was agreed to in tc39/proposal-async-context#18 but that hasn't yet been changed in the spec text.

Copy link
Contributor

@caitp caitp left a 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.

Copy link
Member

@jridgewell jridgewell left a 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() uses undefined this context
  • snapshot.run() restores previous context after abrupt throw


setAndAssertLength(undefined, 0);

setAndAssertLength("42", 42);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@ptomato ptomato added the awaiting stage 2.7 Supports a "Stage 2" feature label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting stage 2.7 Supports a "Stage 2" feature needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants