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.

Sign up for GitHub

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

Jump to bottom

Allow non-void promises in assertThrowsAsync #6052

Merged
merged 8 commits into from
Jun 4, 2020
Merged

Allow non-void promises in assertThrowsAsync #6052

merged 8 commits into from
Jun 4, 2020

Conversation

JonShort
Copy link
Contributor

@JonShort JonShort commented Jun 2, 2020

Fixes #6051

Allows return-type of the Promise to be inferred, or declared.

Before

image

After

image

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ry
Copy link
Member

ry commented Jun 3, 2020

But why would you want to return something from the cb? Unless there's a good reason for this, the complication isn't worth it IMO.

@JonShort
Copy link
Contributor Author

JonShort commented Jun 3, 2020

Thanks for the response! 😁

Here's the context for my issue/PR - promptForEmail reads from stdin and resolves to Promise<string>

It throws if Deno.read fails, or provided email doesn't pass validation

image

If i'm using assertThrowsAsync wrong, or this isn't an issue for others, let me know and I'm happy to close the issue & PR 👍

Thanks for your effort on deno 🦕 😁

@ry
Copy link
Member

ry commented Jun 3, 2020

@JonShort Ok makes sense. Could you please add a test demonstrating assertThrowsAsync with a function that returns a non-void?

@JonShort
Copy link
Contributor Author

JonShort commented Jun 3, 2020

👍 Yeah sure I'll sort this out tonight (I'm on UK-time)

@JonShort JonShort force-pushed the assert-throw-returntype branch from a116f49 to efd08ee Compare June 3, 2020 16:30
@bartlomieju bartlomieju added the std label Jun 3, 2020
@JonShort
Copy link
Contributor Author

JonShort commented Jun 3, 2020

@ry - I've added some tests, let me know if you need anything else 👍

ry
ry reviewed Jun 4, 2020
View reviewed changes
std/testing/asserts_test.ts Outdated Show resolved Hide resolved
ry
ry reviewed Jun 4, 2020
View reviewed changes
std/testing/asserts_test.ts Outdated Show resolved Hide resolved
std/testing/asserts_test.ts Outdated Show resolved Hide resolved
std/testing/asserts_test.ts Outdated Show resolved Hide resolved
ry
ry approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks Jon - sorry for all the nitpicky comments

@ry ry merged commit 4b1638d into denoland:master Jun 4, 2020
@JonShort
Copy link
Contributor Author

JonShort commented Jun 4, 2020

LGTM - thanks Jon - sorry for all the nitpicky comments

No problem! Thanks for all the help, really useful 👍 🦕

@JonShort JonShort deleted the assert-throw-returntype branch June 4, 2020 14:45
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Jun 5, 2020
@JonShort @bartlomieju
feat(std/testing): Allow non-void promises in assertThrowsAsync (deno…
faeea32 
…land#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
@JonShort
feat(std/testing): Allow non-void promises in assertThrowsAsync (deno…
a787490 
…land/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@JonShort
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
4e9bc2a 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@JonShort @caspervonb
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
e8a7dad 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@JonShort @caspervonb
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
a62f373 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@JonShort @caspervonb
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
478a4f7 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@JonShort @caspervonb
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
cce75f4 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@JonShort @caspervonb
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
6455b49 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@JonShort @denobot
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
fcd0de4 
…/deno#6052)
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
@JonShort @denobot
feat(testing): Allow non-void promises in assertThrowsAsync (denoland…
75198c9 
…/deno#6052)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

std/testing/asserts - assertThrowsAsync - allow return types other than Promise<void>
4 participants
@JonShort @CLAassistant @ry @bartlomieju