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

Remove should_panic unit tests #307

Closed
rylev opened this issue Jun 17, 2021 · 2 comments · Fixed by #314
Closed

Remove should_panic unit tests #307

rylev opened this issue Jun 17, 2021 · 2 comments · Fixed by #314
Labels
feature-request This issue requires a new behavior in the product in order be resolved. good first issue This issue tracks work that may be a good starting point for a first-time contributor

Comments

@rylev
Copy link
Contributor

rylev commented Jun 17, 2021

Many unit tests are should_panic tests when they do not have to be.

It is preferable to not use should_panic as this requires string matching against the panic output which can be finicky and does not allow for any type safety.

An example here shows that we are calling unwrap just to match against the panic message. Instead, we should assert_eq! against the error returned.
@rylev rylev added feature-request This issue requires a new behavior in the product in order be resolved. good first issue This issue tracks work that may be a good starting point for a first-time contributor labels Jun 17, 2021
@u5surf
Copy link
Contributor

u5surf commented Jun 19, 2021

@rylev
Hi, I'm newbie as rust programmer.
I would like to try this issue for making improvement my skills.

@rylev
Copy link
Contributor Author

rylev commented Jun 22, 2021

Feel free to submit a PR! Let me know if you need any help! 😊

u5surf added a commit to u5surf/azure-sdk-for-rust that referenced this issue Jun 22, 2021
@u5surf
Remove should_panic unit tests Azure#307
485749d
@u5surf u5surf mentioned this issue Jun 22, 2021
Merged
u5surf added a commit to u5surf/azure-sdk-for-rust that referenced this issue Jun 23, 2021
@u5surf
Fix valiables and format Azure#307
9ab4a69
u5surf added a commit to u5surf/azure-sdk-for-rust that referenced this issue Jun 24, 2021
@u5surf
Remove should_panic unit tests Azure#307
5769d25
u5surf added a commit to u5surf/azure-sdk-for-rust that referenced this issue Jun 24, 2021
@u5surf
Remove should_panic unit tests Azure#307
4daa0e9
rylev pushed a commit that referenced this issue Jun 24, 2021
@u5surf
Remove should_panic unit tests #307 (#314)
29d904b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request This issue requires a new behavior in the product in order be resolved. good first issue This issue tracks work that may be a good starting point for a first-time contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove should_panic unit tests #307 u5surf/azure-sdk-for-rust
2 participants
@rylev @u5surf