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

New style guidelines: should we ban the usage of assertRaises in testing? #1660

Closed
MVrachev opened this issue Nov 3, 2021 · 2 comments · Fixed by #1670
Closed

New style guidelines: should we ban the usage of assertRaises in testing? #1660

MVrachev opened this issue Nov 3, 2021 · 2 comments · Fixed by #1670
Assignees
Labels
backlog Issues to address with priority for current development goals discussion Discussions related to the design, implementation and operation of the project testing
Milestone

Comments

@MVrachev
Copy link
Collaborator

MVrachev commented Nov 3, 2021

Description of issue or feature request:

Currently our code style guidelines doesn't specify anything about the usage of self.assertRaises vs the usage of with self.assertRaises():

Using self.assertRaises can lead to long statements separated to multiline expressions as pointed out by @jku here: #1658 (comment).
On another hand with self.assertRaises(): looks better in my opinion:

with self.assertRaises(DeserializationError):

Long expressions look a lot better
with self.assertRaises(exceptions.UnsignedMetadataError):
when using with self.assertRaises(): than using self.assertRaises(): as pointed out in Jussi's comment.

Current behavior:

No guidelines about self.assertRaises vs with self.assertRaises():.

Expected behavior:

The question is should we specify something more concrete?

EDIT: We decided we would remove all instances of self.assertRaises and replace them with with self.assertRaises.

@MVrachev MVrachev added testing discussion Discussions related to the design, implementation and operation of the project labels Nov 3, 2021
@lukpueh
Copy link
Member

lukpueh commented Nov 3, 2021

I like the recommendation but I'd also like to keep our general code style guide as short as possible.

Maybe this would be a good fit for a test guideline document --> #1129.

@sechkova sechkova added the backlog Issues to address with priority for current development goals label Nov 10, 2021
@sechkova sechkova added this to the Sprint 12 milestone Nov 10, 2021
@MVrachev
Copy link
Collaborator Author

There was a discussion with the maintainers and we decided that we would remove all instances of self.assertRaises and replace them with with self.assertRaises and that if we are going to specify that in the documentation somewhere it will be when solving #1129.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals discussion Discussions related to the design, implementation and operation of the project testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants