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

https://github.com/mlflow/mlflow/blob/fe6618823a2e6038149ee0da675503d2764552ca/tests/store/tracking/test_sqlalchemy_store.py#L107

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

Instructions

https://github.com/mlflow/mlflow/blob/101ad6e8eb383c769178df0df83d1d2a1cea6b4a/pylint_plugins/assert_raises_without_msg.py#L20-L33

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)

Most upvoted comments

Hi @Sumanth077, No. I should’ve informed you but #5914 replaced all the assertRaises calls and closed this issue.