Merge pull request #1746 from cpennington/fix-lms-1532

Fix LMS-1532 (partially instantiated XModules after an exception)

common/lib/xmodule/xmodule/tests/test_error_module.py

@@ -3,10 +3,13 @@ Tests for ErrorModule and NonStaffErrorModule
 """
 import unittest
 from xmodule.tests import get_test_system
-import xmodule.error_module as error_module
+from xmodule.error_module import ErrorDescriptor, ErrorModule, NonStaffErrorDescriptor
 from xmodule.modulestore import Location
-from xmodule.x_module import XModuleDescriptor
-from mock import MagicMock
+from xmodule.x_module import XModuleDescriptor, XModule
+from mock import MagicMock, Mock, patch
+from xblock.runtime import Runtime, UsageStore
+from xblock.field_data import FieldData
+from xblock.fields import ScopeIds
 
 
 class SetupTestErrorModules():
@@ -27,9 +30,9 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules):
         SetupTestErrorModules.setUp(self)
 
     def test_error_module_xml_rendering(self):
-        descriptor = error_module.ErrorDescriptor.from_xml(
+        descriptor = ErrorDescriptor.from_xml(
             self.valid_xml, self.system, self.org, self.course, self.error_msg)
-        self.assertIsInstance(descriptor, error_module.ErrorDescriptor)
+        self.assertIsInstance(descriptor, ErrorDescriptor)
         descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(descriptor, 'student_view').content
         self.assertIn(self.error_msg, context_repr)
@@ -41,9 +44,9 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules):
                                location=self.location,
                                _field_data=self.valid_xml)
 
-        error_descriptor = error_module.ErrorDescriptor.from_descriptor(
+        error_descriptor = ErrorDescriptor.from_descriptor(
             descriptor, self.error_msg)
-        self.assertIsInstance(error_descriptor, error_module.ErrorDescriptor)
+        self.assertIsInstance(error_descriptor, ErrorDescriptor)
         error_descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(error_descriptor, 'student_view').content
         self.assertIn(self.error_msg, context_repr)
@@ -58,12 +61,12 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules):
         SetupTestErrorModules.setUp(self)
 
     def test_non_staff_error_module_create(self):
-        descriptor = error_module.NonStaffErrorDescriptor.from_xml(
+        descriptor = NonStaffErrorDescriptor.from_xml(
             self.valid_xml, self.system, self.org, self.course)
-        self.assertIsInstance(descriptor, error_module.NonStaffErrorDescriptor)
+        self.assertIsInstance(descriptor, NonStaffErrorDescriptor)
 
     def test_from_xml_render(self):
-        descriptor = error_module.NonStaffErrorDescriptor.from_xml(
+        descriptor = NonStaffErrorDescriptor.from_xml(
             self.valid_xml, self.system, self.org, self.course)
         descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(descriptor, 'student_view').content
@@ -76,10 +79,66 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules):
                                location=self.location,
                                _field_data=self.valid_xml)
 
-        error_descriptor = error_module.NonStaffErrorDescriptor.from_descriptor(
+        error_descriptor = NonStaffErrorDescriptor.from_descriptor(
             descriptor, self.error_msg)
-        self.assertIsInstance(error_descriptor, error_module.ErrorDescriptor)
+        self.assertIsInstance(error_descriptor, ErrorDescriptor)
         error_descriptor.xmodule_runtime = self.system
         context_repr = self.system.render(error_descriptor, 'student_view').content
         self.assertNotIn(self.error_msg, context_repr)
         self.assertNotIn(str(descriptor), context_repr)
+
+
+class BrokenModule(XModule):
+    def __init__(self, *args, **kwargs):
+        super(BrokenModule, self).__init__(*args, **kwargs)
+        raise Exception("This is a broken xmodule")
+
+
+class BrokenDescriptor(XModuleDescriptor):
+    module_class = BrokenModule
+
+
+class TestException(Exception):
+    """An exception type to use to verify raises in tests"""
+    pass
+
+
+class TestErrorModuleConstruction(unittest.TestCase):
+    """
+    Test that error module construction happens correctly
+    """
+
+    def setUp(self):
+        field_data = Mock(spec=FieldData)
+        self.descriptor = BrokenDescriptor(
+            Runtime(Mock(spec=UsageStore), field_data),
+            field_data,
+            ScopeIds(None, None, None, 'i4x://org/course/broken/name')
+        )
+        self.descriptor.xmodule_runtime = Runtime(Mock(spec=UsageStore), field_data)
+        self.descriptor.xmodule_runtime.error_descriptor_class = ErrorDescriptor
+        self.descriptor.xmodule_runtime.xmodule_instance = None
+
+    def test_broken_module(self):
+        """
+        Test that when an XModule throws an error during __init__, we
+        get an ErrorModule back from XModuleDescriptor._xmodule
+        """
+        module = self.descriptor._xmodule
+        self.assertIsInstance(module, ErrorModule)
+
+    @patch.object(ErrorDescriptor, '__init__', Mock(side_effect=TestException))
+    def test_broken_error_descriptor(self):
+        """
+        Test that a broken error descriptor doesn't cause an infinite loop
+        """
+        with self.assertRaises(TestException):
+            module = self.descriptor._xmodule
+
+    @patch.object(ErrorModule, '__init__', Mock(side_effect=TestException))
+    def test_broken_error_module(self):
+        """
+        Test that a broken error module doesn't cause an infinite loop
+        """
+        with self.assertRaises(TestException):
+            module = self.descriptor._xmodule

common/lib/xmodule/xmodule/x_module.py

@@ -771,6 +771,10 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
                 )
                 self.xmodule_runtime.xmodule_instance.save()
             except Exception:  # pylint: disable=broad-except
+                # xmodule_instance is set by the XModule.__init__. If we had an error after that,
+                # we need to clean it out so that we can set up the ErrorModule instead
+                self.xmodule_runtime.xmodule_instance = None
+
                 if isinstance(self, self.xmodule_runtime.error_descriptor_class):
                     log.exception('Error creating an ErrorModule from an ErrorDescriptor')
                     raise