Skip to content

Add (currently failing) test case around saving metadata. #514

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
peter-fogg merged 1 commit into from
Jul 29, 2013
Merged

Add (currently failing) test case around saving metadata. #514

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 60 additions & 0 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1359,3 +1359,63 @@ def test_default_metadata_inheritance(self):
self.assertEqual(course.textbooks, fetched_course.textbooks)
# is this test too strict? i.e., it requires the dicts to be ==
self.assertEqual(course.checklists, fetched_course.checklists)


class MetadataSaveTestCase(ModuleStoreTestCase):
"""
Test that metadata is correctly decached.
"""

def setUp(self):
sample_xml = '''
<video display_name="Test Video"
youtube="1.0:p2Q6BrNhdh8,0.75:izygArpw-Qo,1.25:1EeWXzPdhSA,1.5:rABDYkeK0x8"
show_captions="false"
from="00:00:01"
to="00:01:00">
<source src="http://www.example.com/file.mp4"/>
<track src="http://www.example.com/track"/>
</video>
'''
CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])

@singingwolfboy singingwolfboy Jul 29, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to simply call super(ModuleStoreTestCase, self).setUp() rather than duplicating the logic here?

@peter-fogg peter-fogg Jul 29, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This logic isn't present in ModuleStoreTestCase -- the behaviour we get by inheriting is setting up and tearing down the modulestore.

@singingwolfboy singingwolfboy Jul 29, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, what do we gain by subclassing from ModuleStoreTestCase, then? Why not just subclass from unittest.TestCase?

@peter-fogg peter-fogg Jul 29, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We get a unique modulestore collection in mongo, which will get cleaned up after the test is finished.


model_data = {'data': sample_xml}
self.descriptor = ItemFactory.create(parent_location=course_location, category='video', data=model_data)

def test_metadata_persistence(self):
"""
Test that descriptors which set metadata fields in their
constructor are correctly persisted.
"""
# We should start with a source field, from the XML's <source/> tag
self.assertIn('source', own_metadata(self.descriptor))

@sarina sarina Jul 29, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be more comfortable if these were looking for a specific line, eg <source src="http://www.example.com/source.mp4"/>, unless there's a compelling reason I'm wrong?

@peter-fogg peter-fogg Jul 29, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

own_metadata won't be the whole XML string -- it's a dict which is parsed out of the XML attrs. So this isn't a substring check, it's a 'does this key exist in here' test.

@sarina sarina Jul 29, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK. It confused me because the initial string also has the word 'source' in it (source.mp4). I think you should change that to file.mp4 or something so that someone like me, unfamiliar with the test/infrastructure, won't get too confused on first glance.

@peter-fogg peter-fogg Jul 29, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

attrs_to_strip = {
'show_captions',
'youtube_id_1_0',
'youtube_id_0_75',
'youtube_id_1_25',
'youtube_id_1_5',
'start_time',
'end_time',
'source',
'track'
}
# We strip out all metadata fields to reproduce a bug where
# constructors which set their fields (e.g. Video) didn't have
# those changes persisted. So in the end we have the XML data
# in `descriptor.data`, but not in the individual fields
fields = self.descriptor.fields
for field in fields:
if field.name in attrs_to_strip:
field.delete_from(self.descriptor)

# Assert that we correctly stripped the field
self.assertNotIn('source', own_metadata(self.descriptor))
get_modulestore(self.descriptor.location).update_metadata(
self.descriptor.location,
own_metadata(self.descriptor)
)
module = get_modulestore(self.descriptor.location).get_item(self.descriptor.location)
# Assert that get_item correctly sets the metadata
self.assertIn('source', own_metadata(module))