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

assertRejects and assertThrows should return the caught error #2220

Closed
lucacasonato opened this issue May 13, 2022 · 11 comments
Closed

assertRejects and assertThrows should return the caught error #2220

lucacasonato opened this issue May 13, 2022 · 11 comments
Labels
good first issue Good for newcomers suggestion a suggestion yet to be agreed

Comments

@lucacasonato
Copy link
Member

It'd be nice if these functions returned the error they caught, so users could do more intricate assertions on the error itself.

@lucacasonato lucacasonato added the suggestion a suggestion yet to be agreed label May 13, 2022
@bartlomieju
Copy link
Member

Sounds good to me, this should be a simple change.

@bartlomieju bartlomieju added the good first issue Good for newcomers label May 13, 2022
@mrkldshv
Copy link
Contributor

Hey! I'd like to take this issue.

@bartlomieju
Copy link
Member

@mrkldshv please do!

@kt3k
Copy link
Member

kt3k commented May 13, 2022

assertThrows accepts errorCallback as the 2nd arg and you can assert on details about the thrown error in it.

I think we discussed similar suggestion before, but we didn't accept it based on the design principle assertion functions shouldn't return anything ref: #1052 (comment)

@kt3k
Copy link
Member

kt3k commented May 13, 2022

(Note: we added errorCallback overload in #1219)

@mrkldshv
Copy link
Contributor

Right, I've just looked through previous discussion. As pointed by @kt3k, it wasn't added because assertion functions shouldn't return anything. What do you think we should do? @lucacasonato @bartlomieju

@lucacasonato
Copy link
Member Author

I think the "Assertions shouldn't have return values." rule is rather arbitrary. I think in this case returning an error is totally reasonable. the error callback does not really solve the issue for me, because I may need to do some async work with the error

@KyleJune
Copy link
Contributor

In this previous issue I proposed a similar change to having these functions return the values thrown. I believe the fix for this could also be made to be a fix for that issue. Here is a link to my comment there. It would be good to keep error class and message arguments as a shorthand for both asserting that they throw/reject and asserting that it is an Error that was thrown.

#1362 (comment)

@kt3k
Copy link
Member

kt3k commented May 16, 2022

I think the "Assertions shouldn't have return values." rule is rather arbitrary. I think in this case returning an error is totally reasonable. the error callback does not really solve the issue for me, because I may need to do some async work with the error

I'm open to changing that rule now as it's requested/suggested many times and it seems to have some practical use case.

@kitsonk What do you think about this suggested change? Do you have any concerns/objections?

@mrkldshv
Copy link
Contributor

mrkldshv commented Jun 1, 2022

@kt3k Should we close this issue?

@kt3k
Copy link
Member

kt3k commented Jun 1, 2022

@mrkldshv Yes, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers suggestion a suggestion yet to be agreed
Projects
None yet
Development

No branches or pull requests

5 participants