assertRaises(Regex)?(Exception) is problematic

[sobolevn]

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:

1. Use custom exception classes where possible class MyExc(Exception): ..., this way we will have 100% guarantee that the test is accurate
2. Use assertRaisesRegex more, where possible
3. 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
4. If as exc is used and there are isinstance checks - then we can keep it as is
5. Keep it, where literally any exception can happen (or different ones)
6. 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

• gh-106300: Improve assertRaises(Exception) usages in tests #106302
• [3.12] gh-106300: Improve assertRaises(Exception) usages in tests (GH-106302) #106534
• [3.11] gh-106300: Improve assertRaises(Exception) usages in tests (GH-106302). #106545
• gh-106300: Improve assertRaises(Exception) usage in test_runner #106737
• [3.12] gh-106300: Improve errors testing in test_unittest.test_runner (GH-106737) #108006
• [3.11] gh-106300: Improve errors testing in test_unittest.test_runner (GH-106737) #108007

[sobolevn]

And I found a real bug hidden under assertRaises(Exception) :)

This test

cpython/Lib/test/test_unittest/test_runner.py

Lines 389 to 404 in 04dfc6f

	def test_doClassCleanups_with_errors_addClassCleanUp(self):
	class TestableTest(unittest.TestCase):
	def testNothing(self):
	pass
	def cleanup1():
	raise Exception('cleanup1')
	def cleanup2():
	raise Exception('cleanup2')
	TestableTest.addClassCleanup(cleanup1)
	TestableTest.addClassCleanup(cleanup2)
	with self.assertRaises(Exception) as e:
	TestableTest.doClassCleanups()
	self.assertEqual(e, 'cleanup1')

does something interesting:

with self.assertRaises(Exception) as e:
    TestableTest.doClassCleanups()
    self.assertEqual(e, 'cleanup1')

Reader might expect that TestableTest.doClassCleanups() raises some kind of exception, but it does not. What really happens is that e is not equal to 'cleanup1' and AssertionError happens inside self.assertRaises(Exception) and the tests passes successfully, but it is still broken!

I will fix unittest tests in a separate PR. There are quite a lot of usages of this pattern.

[serhiy-storchaka]

Nice catch. Could you please fix also cases with assertRaises(BaseException)?

[sobolevn]

Yes, sure. Is it better to include BaseException changes into the existing PR or into a new one?

[serhiy-storchaka]

Up to you.

[hugovk]

Thanks for the fixes!