Skip to content

Ensure that the original error gets preserved by the ErrorModule #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dianakhuang merged 2 commits into from
Jun 5, 2013
Merged

Ensure that the original error gets preserved by the ErrorModule #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3b36b8
Use from_descriptor instead of from_xml to make ErrorDescriptor
dianakhuang May 31, 2013
d895c64
Use full descriptor when showing errors, not just the model data.
dianakhuang Jun 4, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/lib/xmodule/xmodule/error_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def from_json(cls, json_data, system, error_msg='Error not available'):
def from_descriptor(cls, descriptor, error_msg='Error not available'):
return cls._construct(
descriptor.system,
descriptor._model_data,
str(descriptor),
error_msg,
location=descriptor.location,
)
Expand Down
52 changes: 40 additions & 12 deletions common/lib/xmodule/xmodule/tests/test_error_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
import unittest
from xmodule.tests import test_system
import xmodule.error_module as error_module
from xmodule.modulestore import Location
from xmodule.x_module import XModuleDescriptor
from mock import MagicMock


class TestErrorModule(unittest.TestCase):
Expand All @@ -14,22 +17,33 @@ def setUp(self):
self.system = test_system()
self.org = "org"
self.course = "course"
self.fake_xml = "<problem />"
self.location = Location(['i4x', self.org, self.course, None, None])
self.valid_xml = "<problem />"
self.broken_xml = "<problem>"
self.error_msg = "Error"

def test_error_module_create(self):
def test_error_module_xml_rendering(self):
descriptor = error_module.ErrorDescriptor.from_xml(
self.fake_xml, self.system, self.org, self.course)
self.valid_xml, self.system, self.org, self.course, self.error_msg)
self.assertTrue(isinstance(descriptor, error_module.ErrorDescriptor))

def test_error_module_rendering(self):
descriptor = error_module.ErrorDescriptor.from_xml(
self.fake_xml, self.system, self.org, self.course, self.error_msg)
module = descriptor.xmodule(self.system)
rendered_html = module.get_html()
self.assertIn(self.error_msg, rendered_html)
self.assertIn(self.fake_xml, rendered_html)
self.assertIn(self.valid_xml, rendered_html)

def test_error_module_from_descriptor(self):
descriptor = MagicMock([XModuleDescriptor],
system=self.system,
location=self.location,
_model_data=self.valid_xml)

error_descriptor = error_module.ErrorDescriptor.from_descriptor(
descriptor, self.error_msg)
self.assertTrue(isinstance(error_descriptor, error_module.ErrorDescriptor))
module = error_descriptor.xmodule(self.system)
rendered_html = module.get_html()
self.assertIn(self.error_msg, rendered_html)
self.assertIn(str(descriptor), rendered_html)


class TestNonStaffErrorModule(TestErrorModule):
Expand All @@ -39,13 +53,27 @@ class TestNonStaffErrorModule(TestErrorModule):

def test_non_staff_error_module_create(self):
descriptor = error_module.NonStaffErrorDescriptor.from_xml(
self.fake_xml, self.system, self.org, self.course)
self.valid_xml, self.system, self.org, self.course)
self.assertTrue(isinstance(descriptor, error_module.NonStaffErrorDescriptor))

def test_non_staff_error_module_rendering(self):
def test_from_xml_render(self):
descriptor = error_module.NonStaffErrorDescriptor.from_xml(
self.fake_xml, self.system, self.org, self.course)
self.valid_xml, self.system, self.org, self.course)
module = descriptor.xmodule(self.system)
rendered_html = module.get_html()
self.assertNotIn(self.error_msg, rendered_html)
self.assertNotIn(self.fake_xml, rendered_html)
self.assertNotIn(self.valid_xml, rendered_html)
Copy link
Contributor

@wedaly wedaly Jun 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we expect to be included in the rendered html? A generic error message? If so, we should assert so explicitly here.

Copy link
Contributor Author

@dianakhuang dianakhuang Jun 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with that is that the current test system renderer just spits back out the original context. What do you recommend that I do? Change it to render using mitxmako?

Copy link
Contributor

@wedaly wedaly Jun 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, that makes sense. Thanks for the explanation.


def test_error_module_from_descriptor(self):
descriptor = MagicMock([XModuleDescriptor],
system=self.system,
location=self.location,
_model_data=self.valid_xml)

error_descriptor = error_module.NonStaffErrorDescriptor.from_descriptor(
descriptor, self.error_msg)
self.assertTrue(isinstance(error_descriptor, error_module.ErrorDescriptor))
module = error_descriptor.xmodule(self.system)
rendered_html = module.get_html()
self.assertNotIn(self.error_msg, rendered_html)
self.assertNotIn(str(descriptor), rendered_html)
5 changes: 2 additions & 3 deletions lms/djangoapps/courseware/module_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,8 @@ def can_execute_unsafe_code():
else:
err_descriptor_class = NonStaffErrorDescriptor

err_descriptor = err_descriptor_class.from_xml(
str(descriptor), descriptor.system,
org=descriptor.location.org, course=descriptor.location.course,
err_descriptor = err_descriptor_class.from_descriptor(
descriptor,
error_msg=exc_info_to_str(sys.exc_info())
)
Copy link
Contributor

@brianhw brianhw May 31, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the methods, doesn't look quite equivalent. In particular, the preceding approach results in a bogus location. Don't know if that's desirable -- it might be an improvement to actually store the original location, but it might also break things if the location is used to then find the corresponding descriptor, and yields the original instead of none. I'm willing to gamble on that.

But using the from_descriptor ends up using descriptor._model_data instead of str(descriptor). Any idea how different those are?

Copy link
Contributor Author

@dianakhuang dianakhuang May 31, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference is that the descriptor dumps the location and the module type, as far as I can tell, at least for capa problems and videos. It is useful to have the location show up in the error message, I agree, but maybe the raw data section is not the best place for it. I'll talk to @shnayder about what we really want this to do.

Copy link
Contributor

@wedaly wedaly Jun 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there integration tests for this line? I thought we had some, but I can't seem to find them.

The place for these would be in courseware/tests/test_module_render.py. The current state of those tests are not good (uses an XML-based course as a fixture, which is a practice we're moving away from). Still, we should have a test that when I call get_module_for_descriptor() and an error condition occurs, I get the appropriate error module.

get_module_for_descriptor() strikes me as a prime candidate to refactor for testability. Maybe we could use this for the group testing discussion we talked about?


Expand Down