mlflow: Replace `unittest.TestCase.assertRaises` with `unittest.TestCase.assertRaisesRegex`
Some tests use unittest.TestCase.assertRaises to test an exception is raised for illegal operations, but they need to be replaces with unittest.TestCase.assertRaisesRegex.
Why do we need this change?
Let’s say we have a function that raises an exception:
def throw_exception(...):
if condition_1:
raise TypeError("condition_1")
if condition_2:
raise TypeError("condition_2")
...
If we test this function using assertRaises:
class MyTest(unittest.TestCase):
def test_throw_exception(self):
# Does `throw_exception` really raise the second TypeError?
# It might throw the first TypeError, then the test will pass.
with self.assertRaises(TypeError):
throw_exception(...) # should raise TypeError("condition_2")
If we test this function using assertRaisesRegex:
class MyTest(unittest.TestCase):
def test_throw_exception(self):
# This test fails when `throw_exception` raises the first TypeError.
with self. assertRaisesRegex(TypeError, "condition_b"):
throw_exception(...) # should raise TypeError("condition_2")
Example
The code above needs to be fixed to the following:
# "<string that matches the error message>" must be replaced
with self.assertRaisesRegex(MlflowException, "<string that matches the error message>") as e:
References
- https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises
- https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaisesRegex
Instructions
Ping me with the file you want to work on 😃
| File | Assignee | PR | Done |
|---|---|---|---|
tests/entities/test_run_status.py |
@Sumanth077 | ||
tests/store/model_registry/test_sqlalchemy_store.py |
@ognis1205 | #5875 | ✅ |
tests/store/db/test_utils.py |
@erich-db | ||
tests/store/tracking/__init__.py |
@Sumanth077 | ||
tests/store/tracking/test_file_store.py |
@andy1122 | ||
tests/store/tracking/test_sqlalchemy_store.py |
@ognis1205 | #5875 | ✅ |
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 18 (15 by maintainers)
Hi @Sumanth077, No. I should’ve informed you but #5914 replaced all the
assertRaisescalls and closed this issue.