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

Implementation of RFC: Async Helpers (#3724) #3728

Merged
merged 12 commits into from
Feb 21, 2023

Conversation

cjtenny
Copy link
Contributor

@cjtenny cjtenny commented Nov 23, 2022

This implements the harness/asyncHelpers.js file proposed in @ptomato 's RFC, with its functions asyncTest(testFunc) and assert.throwsAsync(expectedErrorConstructor, funcOrThenable, message). Also included are tests for both, including tests mirroring assert.throws and a number of additional throwsAsync-specific tests.

Per @jugglinmike 's comments in the maintainers call, never uses await; always explicitly invokes as a thenable.

Implements the original intention of accepting a function or thenable for throwsAsync, happy to modify if there's some general agreement on changing that.

harness/asyncHelpers.js Outdated Show resolved Hide resolved
harness/asyncHelpers.js Outdated Show resolved Hide resolved
harness/asyncHelpers.js Outdated Show resolved Hide resolved
@Ms2ger Ms2ger mentioned this pull request Dec 1, 2022
@cjtenny cjtenny force-pushed the rfc-async-helpers-impl branch 2 times, most recently from 0b19ce3 to e776c25 Compare December 31, 2022 01:22
@cjtenny cjtenny mentioned this pull request Jan 19, 2023
@cjtenny cjtenny marked this pull request as ready for review January 19, 2023 23:40
@cjtenny cjtenny requested a review from a team as a code owner January 19, 2023 23:40
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Now that the RFC is merged I went over this in a bit more detail, and found a few things that should be fixed or clarified. Also it's slightly odd that the CI didn't run, so it would be good to find out why that is.

@cjtenny
Copy link
Contributor Author

cjtenny commented Feb 10, 2023

Rebased and addressed feedback; feedback addressed in separate commit, nothing else changed in older commits. I'm sorry I dropped the ball on this and hadn't responded sooner.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Just some minor loose ends!

Before merging would you mind debugging why the CI is not running? It looks like you may have to authorize the CircleCI app on your fork or something.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ptomato ptomato merged commit d216cc1 into tc39:main Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants