Fix SynchronousThreadPoolExecutorFixture mock of Future

The fixture is not mocking the Future.result() method correctly and an
attempt to access it results in the following error:

  AttributeError: Mock object has no attribute 'result'

Instead of mocking the submit() method, we can just swap out the
GreenThreadPoolExecutor for the SynchronousExecutor to make the
executor synchronous for testing.

A few changes were also needed in unit tests that are running with the
thread pool executor. Because the thread pool executor is created in
the compute manager's __init__ method, we need to patch the synchronous
executor in before calling the base test class's setUp method. Next, a
couple of unit tests were asserting that an exception would be raised
from the thread pool executor's submit() method but that will not
happen because if an exception is raised during a submitted task, that
exception will not be raised until Future.result() is called. And
Future.result() is never called in the compute manager.

Closes-Bug: #1823278

Change-Id: I719c222a5fa3d654e7397f126a1338e653b7bbcc

nova/tests/fixtures.py

@@ -1106,24 +1106,22 @@ class SpawnIsSynchronousFixture(fixtures.Fixture):
             'nova.utils.spawn', _FakeGreenThread))
 
 
-class SynchronousThreadPoolExecutorFixture(fixtures.Fixture):
-    """Make GreenThreadPoolExecutor.submit() synchronous.
+class _FakeExecutor(futurist.SynchronousExecutor):
+    def __init__(self, *args, **kwargs):
+        # Ignore kwargs (example: max_workers) that SynchronousExecutor
+        # does not support.
+        super(_FakeExecutor, self).__init__()
 
-    The function passed to submit() will be executed and a mock.Mock
-    object will be returned as the Future where Future.result() will
-    return the result of the call to the submitted function.
+
+class SynchronousThreadPoolExecutorFixture(fixtures.Fixture):
+    """Make GreenThreadPoolExecutor synchronous.
+
+    Replace the GreenThreadPoolExecutor with the SynchronousExecutor.
     """
     def setUp(self):
         super(SynchronousThreadPoolExecutorFixture, self).setUp()
-
-        def fake_submit(_self, fn, *args, **kwargs):
-            result = fn(*args, **kwargs)
-            future = mock.Mock(spec=futurist.Future)
-            future.return_value.result.return_value = result
-            return future
         self.useFixture(fixtures.MonkeyPatch(
-            'futurist.GreenThreadPoolExecutor.submit',
-            fake_submit))
+            'futurist.GreenThreadPoolExecutor', _FakeExecutor))
 
 
 class BannedDBSchemaOperations(fixtures.Fixture):

nova/tests/unit/compute/test_compute.py

@@ -1509,9 +1509,12 @@ class ComputeTestCase(BaseTestCase,
                       test_diagnostics.DiagnosticsComparisonMixin,
                       fake_resource_tracker.RTMockMixin):
     def setUp(self):
+        # This needs to go before we call setUp because the thread pool
+        # executor is created in ComputeManager.__init__, which is called
+        # during setUp.
+        self.useFixture(fixtures.SynchronousThreadPoolExecutorFixture())
         super(ComputeTestCase, self).setUp()
         self.useFixture(fixtures.SpawnIsSynchronousFixture())
-        self.useFixture(fixtures.SynchronousThreadPoolExecutorFixture())
 
         self.image_api = image_api.API()
         self.default_flavor = objects.Flavor.get_by_name(self.context,
@@ -6298,11 +6301,13 @@ class ComputeTestCase(BaseTestCase,
                     'resources': mock.sentinel.allocs,
                 }
             }
-            self.assertRaises(test.TestingException,
-                              self.compute.live_migration,
-                              c, dest=dest_host, block_migration=True,
-                              instance=instance, migration=migration,
-                              migrate_data=migrate_data)
+            # This call will not raise TestingException because if a task
+            # submitted to a thread executor raises, the exception will not be
+            # raised unless Future.result() is called.
+            self.compute.live_migration(
+                    c, dest=dest_host, block_migration=True,
+                    instance=instance, migration=migration,
+                    migrate_data=migrate_data)
             mock_setup.assert_called_once_with(c, instance, self.compute.host)
             mock_client.move_allocations.assert_called_once_with(
                 c, migration.uuid, instance.uuid)
@@ -6414,8 +6419,10 @@ class ComputeTestCase(BaseTestCase,
                                'cleanup') as mock_cleanup:
             mock_cleanup.side_effect = test.TestingException
 
-            self.assertRaises(test.TestingException,
-                self.compute.live_migration,
+            # This call will not raise TestingException because if a task
+            # submitted to a thread executor raises, the exception will not be
+            # raised unless Future.result() is called.
+            self.compute.live_migration(
                 c, dest, instance, False, migration, migrate_data)
 
         # ensure we have updated the instance and migration objects

nova/tests/unit/test_fixtures.py

@@ -18,6 +18,7 @@ import copy
 import sys
 
 import fixtures as fx
+import futurist
 import mock
 from oslo_config import cfg
 from oslo_db import exception as db_exc
@@ -414,6 +415,17 @@ class TestSpawnIsSynchronousFixture(testtools.TestCase):
         self.assertEqual(1, len(call_count))
 
 
+class TestSynchronousThreadPoolExecutorFixture(testtools.TestCase):
+    def test_submit_passes_through(self):
+        self.useFixture(fixtures.SynchronousThreadPoolExecutorFixture())
+        tester = mock.MagicMock()
+        executor = futurist.GreenThreadPoolExecutor()
+        future = executor.submit(tester.function, 'foo', bar='bar')
+        tester.function.assert_called_once_with('foo', bar='bar')
+        result = future.result()
+        self.assertEqual(tester.function.return_value, result)
+
+
 class TestBannedDBSchemaOperations(testtools.TestCase):
     def test_column(self):
         column = sqlalchemy.Column()