Skip to content

Do not use ZeroDivisionError in tests for unrelated things #119989

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

Closed
sobolevn opened this issue Jun 3, 2024 · 4 comments
Closed

Do not use ZeroDivisionError in tests for unrelated things #119989

sobolevn opened this issue Jun 3, 2024 · 4 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@sobolevn
Copy link
Member

sobolevn commented Jun 3, 2024

Feature or enhancement

Refs #119066

I'm a little horrified at how often we use ZeroDivisionError in random tests that have nothing to do with arithmetic, but that's another story ...

Probably, most of them should be converted to some custom exception.

@sobolevn sobolevn added type-feature A feature request or enhancement tests Tests in the Lib/test dir labels Jun 3, 2024
@terryjreedy
Copy link
Member

terryjreedy commented Jun 3, 2024

Specifically, #119066 (review).
@mdickinson What are examples of horror-inducing uses?

@serhiy-storchaka
Copy link
Member

1/0 is just the simplest way to raise a rare exception. We can use raise OtherRareException or even raise CustomError, but this is more verbose.

@terryjreedy
Copy link
Member

To answer my question: I routinely use something like a a to get a SyntaxError and specifically 1/0' for a 'generic' runtime error. That said, greppping Lib/test recursively for ZeroDivisionError` gets 524 hits. Some test classes raise the long name itself. For instance:

# test_iter, 83-5.
class BadIterableClass:
    def __iter__(self):
        raise ZeroDivisionError

Others do not:

# test_xml_etree, 2790-2.
class BadElementPath(str):
    def __eq__(self, o):
        raise 1/0

Regardless, the multiple tests using 'Bad' classs require the long error name. I could be persuaded that defining something like class FakeErr, raising that in the Bad class, and checking for the short name in tests would be clearer, as well as involving less writing and reading.

This strikes me as a test style proposal that is plausible but needs discussion on Discord or Discourse before possibly revising PEP8 or Devguide.

Serhiy, only a few in test_tkinter:

test_tkinter\test_colorchooser.py: 49:             raise ZeroDivisionError
test_tkinter\test_colorchooser.py: 53:             self.assertRaises(ZeroDivisionError, askcolor)
test_tkinter\test_colorchooser.py: 59:             self.assertRaises(ZeroDivisionError, askcolor)
test_tkinter\test_messagebox.py: 18:             raise ZeroDivisionError
test_tkinter\test_messagebox.py: 22:             self.assertRaises(ZeroDivisionError, showinfo, "Spam", "Egg Information")
test_tkinter\test_messagebox.py: 27:             self.assertRaises(ZeroDivisionError, showinfo, "Spam", "Egg Information")

@mdickinson
Copy link
Member

@terryjreedy @sobolevn I apologise for causing unnecessary work and brain cycles here; I think I shouldn't have made the original comment, and definitely shouldn't have used the word "horrified".

I do think that this is a (very minor) abuse of ZeroDivisionError, but my complaint is very much at nitpick level. That is, if I were reviewing a PR that used raise ZeroDivisionError I might suggest introducing and using a custom exception, but I don't think it's worth changing existing cases of this. We have better ways to spend core developer cycles.

@sobolevn sobolevn closed this as not planned Won't fix, can't repro, duplicate, stale Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants