assertRaises(Exception) is problematic, because it does not really test anything.
For example, here's the test code from test_mailbox.py:
def raiser(*args, **kw):
raise Exception("a fake error")
support.patch(self, email.generator.BytesGenerator, 'flatten', raiser)
with self.assertRaises(Exception):
self._box.add(email.message_from_string("From: Alphöso"))self.assertRaises(Exception) can happen for any other reason, we have zero confidence in the fact that we have the right exception. For example, a simple typo in email.message_from_stringX will also satisfy this check.
Sometimes, it is a bit better when assertRaisesRegex is used, then we have at least have a message to compare and be at least partially sure that the exception is correct. But, it is still not quite good, because we don't know the type of error that happen and it might accidentally change.
There are multiple places where this pattern is used:
Lib/test/test_ordered_dict.py
189: with self.assertRaises(Exception):
Lib/test/test_importlib/test_main.py
71: with self.assertRaises(Exception):
Lib/test/test_code.py
182: with self.assertRaises(Exception):
Lib/test/test_shutil.py
2741: with self.assertRaises(Exception):
Lib/test/test_unittest/testmock/testasync.py
464: with self.assertRaises(Exception):
Lib/test/test_unittest/test_runner.py
349: with self.assertRaises(Exception) as cm:
372: with self.assertRaises(Exception) as cm:
382: with self.assertRaises(Exception) as cm:
402: with self.assertRaises(Exception) as e:
633: with self.assertRaises(Exception) as e:
858: with self.assertRaises(Exception) as cm:
891: with self.assertRaises(Exception) as cm:
902: with self.assertRaises(Exception) as cm:
Lib/test/test_concurrent_futures.py
1039: with self.assertRaises(Exception) as cm:
Lib/test/test_mailbox.py
122: with self.assertRaises(Exception):
Lib/test/test_yield_from.py
1121: with self.assertRaises(Exception) as caught:
1385: with self.assertRaises(Exception) as caught:
1397: with self.assertRaises(Exception) as caught:
1411: with self.assertRaises(Exception) as caught:
1423: with self.assertRaises(Exception) as caught:
1435: with self.assertRaises(Exception) as caught:
1507: with self.assertRaises(Exception) as caught:
Lib/test/test_email/test_message.py
706: with self.assertRaises(Exception) as ar:
Lib/test/test_codecs.py
2825: with self.assertRaises(Exception) as failure:
2832: with self.assertRaises(Exception) as failure:
Lib/test/test_mmap.py
703: with self.assertRaises(Exception) as exc:
Lib/test/test_tarfile.py
2801: with self.assertRaises(Exception) as exc:
I suggest to:
- Use custom exception classes where possible
class MyExc(Exception): ..., this way we will have 100% guarantee that the test is accurate
- Use
assertRaisesRegex more, where possible
- Keep some of the
assertRaises(Exception) where it is legit, for example test_ordereddict says: Note, the exact exception raised is not guaranteed. The only guarantee that the next() will not succeed
- If
as exc is used and there are isinstance checks - then we can keep it as is
- Keep it, where literally any exception can happen (or different ones)
- Keep it where
Exception class is tested excplicitly, like in test_yield_from with g.throw(Exception)
I will send a PR with the fix.
Linked PRs
assertRaises(Exception)is problematic, because it does not really test anything.For example, here's the test code from
test_mailbox.py:self.assertRaises(Exception)can happen for any other reason, we have zero confidence in the fact that we have the right exception. For example, a simple typo inemail.message_from_stringXwill also satisfy this check.Sometimes, it is a bit better when
assertRaisesRegexis used, then we have at least have a message to compare and be at least partially sure that the exception is correct. But, it is still not quite good, because we don't know the type of error that happen and it might accidentally change.There are multiple places where this pattern is used:
I suggest to:
class MyExc(Exception): ..., this way we will have 100% guarantee that the test is accurateassertRaisesRegexmore, where possibleassertRaises(Exception)where it is legit, for exampletest_ordereddictsays:Note, the exact exception raised is not guaranteed. The only guarantee that the next() will not succeedas excis used and there areisinstancechecks - then we can keep it as isExceptionclass is tested excplicitly, like intest_yield_fromwithg.throw(Exception)I will send a PR with the fix.
Linked PRs
assertRaises(Exception)usages in tests #106302assertRaises(Exception)usages in tests (GH-106302) #106534assertRaises(Exception)usages in tests (GH-106302). #106545assertRaises(Exception)usage intest_runner#106737