From c82dba22c24cd7cd4d7ce3629da24644414cb374 Mon Sep 17 00:00:00 2001
From: Johannes Erdfelt <johannes.erdfelt@rackspace.com>
Date: Thu, 20 Sep 2012 17:19:05 +0000
Subject: Clarify dangerous use of exceptions in unit tests

Also, update unit tests to use more specific exception where possible
to make auditing easier in the future.

Change-Id: I4906e2f8e3ebacf0587f1471ca8dd46c73edc17a
---
 nova/testing/README.rst                 | 30 +++++++++++++++++++++++++-----
 nova/tests/api/openstack/test_common.py | 12 ++++--------
 nova/tests/compute/test_compute.py      |  4 ++--
 nova/tests/test_db_api.py               |  3 +--
 4 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/nova/testing/README.rst b/nova/testing/README.rst
index 67fa33d1d..4c341b7ed 100644
--- a/nova/testing/README.rst
+++ b/nova/testing/README.rst
@@ -54,13 +54,33 @@ Writing Integration Tests
 
 TBD
 
-Tests and assertRaises
-----------------------
-When asserting that a test should raise an exception, test against the
-most specific exception possible. An overly broad exception type (like
-Exception) can mask errors in the unit test itself.
+Tests and Exceptions
+--------------------
+A properly written test asserts that particular behavior occurs. This can
+be a success condition or a failure condition, including an exception.
+When asserting that a particular exception is raised, the most specific
+exception possible should be used.
+
+In particular, testing for Exception being raised is almost always a
+mistake since it will match (almost) every exception, even those
+unrelated to the exception intended to be tested.
+
+This applies to catching exceptions manually with a try/except block,
+or using assertRaises().
 
 Example::
 
     self.assertRaises(exception.InstanceNotFound, db.instance_get_by_uuid,
                       elevated, instance_uuid)
+
+If a stubbed function/method needs a generic exception for testing
+purposes, test.TestingException is available.
+
+Example::
+
+    def stubbed_method(self):
+        raise test.TestingException()
+    self.stubs.Set(cls, 'inner_method', stubbed_method)
+
+    obj = cls()
+    self.assertRaises(test.TestingException, obj.outer_method)
diff --git a/nova/tests/api/openstack/test_common.py b/nova/tests/api/openstack/test_common.py
index 250a199e8..4ebd49ca2 100644
--- a/nova/tests/api/openstack/test_common.py
+++ b/nova/tests/api/openstack/test_common.py
@@ -290,10 +290,8 @@ class MiscFunctionsTest(test.TestCase):
         try:
             common.raise_http_conflict_for_instance_invalid_state(exc,
                     'meow')
-        except Exception, e:
-            self.assertTrue(isinstance(e, webob.exc.HTTPConflict))
-            msg = str(e)
-            self.assertEqual(msg,
+        except webob.exc.HTTPConflict as e:
+            self.assertEqual(unicode(e),
                 "Cannot 'meow' while instance is in fake_attr fake_state")
         else:
             self.fail("webob.exc.HTTPConflict was not raised")
@@ -303,10 +301,8 @@ class MiscFunctionsTest(test.TestCase):
         try:
             common.raise_http_conflict_for_instance_invalid_state(exc,
                     'meow')
-        except Exception, e:
-            self.assertTrue(isinstance(e, webob.exc.HTTPConflict))
-            msg = str(e)
-            self.assertEqual(msg,
+        except webob.exc.HTTPConflict as e:
+            self.assertEqual(unicode(e),
                 "Instance is in an invalid state for 'meow'")
         else:
             self.fail("webob.exc.HTTPConflict was not raised")
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 63af33153..747774fa4 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -2264,7 +2264,7 @@ class ComputeTestCase(BaseTestCase):
 
         try:
             raise NotImplementedError('test')
-        except Exception:
+        except NotImplementedError:
             exc_info = sys.exc_info()
 
         self.stubs.Set(nova.db, 'instance_fault_create', fake_db_fault_create)
@@ -2292,7 +2292,7 @@ class ComputeTestCase(BaseTestCase):
 
         try:
             raise user_exc
-        except Exception:
+        except exception.Invalid:
             exc_info = sys.exc_info()
 
         self.stubs.Set(nova.db, 'instance_fault_create', fake_db_fault_create)
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 641c7c8cd..e434faaad 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -46,11 +46,10 @@ class DbApiTestCase(test.TestCase):
         return db.instance_create(self.context, args)
 
     def test_ec2_ids_not_found_are_printable(self):
-
         def check_exc_format(method):
             try:
                 method(self.context, 'fake')
-            except Exception as exc:
+            except exception.NotFound as exc:
                 self.assertTrue('fake' in unicode(exc))
 
         check_exc_format(db.get_ec2_volume_id_by_uuid)
-- 
cgit