Clean up quality violations in split_test module

common/djangoapps/user_api/user_service.py

@@ -14,6 +14,7 @@ from user_api.models import UserCourseTag
 # global tags (e.g. using the existing UserPreferences table))
 COURSE_SCOPE = 'course'
 
+
 def get_course_tag(user, course_id, key):
     """
     Gets the value of the user's course tag for the specified key in the specified

common/lib/xmodule/xmodule/partitions/test_partitions.py

@@ -5,7 +5,7 @@ Test the partitions and partitions service
 
 from collections import defaultdict
 from unittest import TestCase
-from mock import Mock, MagicMock
+from mock import Mock
 
 from xmodule.partitions.partitions import Group, UserPartition
 from xmodule.partitions.partitions_service import PartitionService

common/lib/xmodule/xmodule/split_test_module.py

@@ -40,7 +40,7 @@ class SplitTestFields(object):
     )
 
 
-@XBlock.needs('user_tags')
+@XBlock.needs('user_tags')  # pylint: disable=abstract-method
 @XBlock.needs('partitions')
 class SplitTestModule(SplitTestFields, XModule):
     """
@@ -196,7 +196,7 @@ class SplitTestModule(SplitTestFields, XModule):
         return progress
 
 
-@XBlock.needs('user_tags')
+@XBlock.needs('user_tags')  # pylint: disable=abstract-method
 @XBlock.needs('partitions')
 class SplitTestDescriptor(SplitTestFields, SequenceDescriptor):
     # the editing interface can be the same as for sequences -- just a container
@@ -223,4 +223,3 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor):
         makes it use module.get_child_descriptors().
         """
         return True
-

common/lib/xmodule/xmodule/tests/test_split_module.py

@@ -44,9 +44,10 @@ class SplitTestModuleTest(XModuleXmlImportTest):
         self.module_system = get_test_system()
 
         def get_module(descriptor):
+            """Mocks module_system get_module function"""
             module_system = get_test_system()
             module_system.get_module = get_module
-            descriptor.bind_for_student(module_system, descriptor._field_data)
+            descriptor.bind_for_student(module_system, descriptor._field_data)  # pylint: disable=protected-access
             return descriptor
 
         self.module_system.get_module = get_module
@@ -67,8 +68,7 @@ class SplitTestModuleTest(XModuleXmlImportTest):
         self.module_system._services['partitions'] = self.partitions_service  # pylint: disable=protected-access
 
         self.split_test_module = course_seq.get_children()[0]
-        self.split_test_module.bind_for_student(self.module_system, self.split_test_module._field_data)
-
+        self.split_test_module.bind_for_student(self.module_system, self.split_test_module._field_data)  # pylint: disable=protected-access
 
     @ddt.data(('0', 'split_test_cond0'), ('1', 'split_test_cond1'))
     @ddt.unpack
@@ -83,7 +83,7 @@ class SplitTestModuleTest(XModuleXmlImportTest):
 
     @ddt.data(('0',), ('1',))
     @ddt.unpack
-    def test_child_old_tag_value(self, user_tag):
+    def test_child_old_tag_value(self, _user_tag):
         # If user_tag has a stale value, we should still get back a valid child url
         self.tags_service.set_tag(
             self.tags_service.COURSE_SCOPE,
@@ -109,13 +109,13 @@ class SplitTestModuleTest(XModuleXmlImportTest):
 
     @ddt.data(('0',), ('1',))
     @ddt.unpack
-    def test_child_missing_tag_value(self, user_tag):
+    def test_child_missing_tag_value(self, _user_tag):
         # If user_tag has a missing value, we should still get back a valid child url
         self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1'])
 
     @ddt.data(('100',), ('200',), ('300',), ('400',), ('500',), ('600',), ('700',), ('800',), ('900',), ('1000',))
     @ddt.unpack
-    def test_child_persist_new_tag_value_when_tag_missing(self, user_tag):
+    def test_child_persist_new_tag_value_when_tag_missing(self, _user_tag):
         # If a user_tag has a missing value, a group should be saved/persisted for that user.
         # So, we check that we get the same url_name when we call on the url_name twice.
         # We run the test ten times so that, if our storage is failing, we'll be most likely to notice it.

common/lib/xmodule/xmodule/tests/xml/factories.py

@@ -146,6 +146,7 @@ class SequenceFactory(XmlImportFactory):
     """Factory for <sequential> nodes"""
     tag = 'sequential'
 
+
 class VerticalFactory(XmlImportFactory):
     """Factory for <vertical> nodes"""
     tag = 'vertical'

lms/djangoapps/courseware/tests/test_split_module.py

 """
 Test for split test XModule
 """
-import ddt
-from mock import MagicMock, patch, Mock
 from django.core.urlresolvers import reverse
 from django.test.utils import override_settings
 
-from student.tests.factories import UserFactory, CourseEnrollmentFactory, AdminFactory
+from student.tests.factories import UserFactory, CourseEnrollmentFactory
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from xmodule.partitions.partitions import Group, UserPartition
-from xmodule.partitions.test_partitions import StaticPartitionService
 from user_api.tests.factories import UserCourseTagFactory
-from xmodule.partitions.partitions import Group, UserPartition
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class SplitTestBase(ModuleStoreTestCase):
+    """
+    Sets up a basic course and user for split test testing.
+    Also provides tests of rendered HTML for two user_tag conditions, 0 and 1.
+    """
     __test__ = False
+    COURSE_NUMBER = 'split-test-base'
+    ICON_CLASSES = None
+    TOOLTIPS = None
+    HIDDEN_CONTENT = None
+    VISIBLE_CONTENT = None
 
     def setUp(self):
         self.partition = UserPartition(
@@ -53,6 +58,10 @@ class SplitTestBase(ModuleStoreTestCase):
         self.client.login(username=self.student.username, password='test')
 
     def _video(self, parent, group):
+        """
+        Returns a video component with parent ``parent``
+        that is intended to be displayed to group ``group``.
+        """
         return ItemFactory.create(
             parent_location=parent.location,
             category="video",
@@ -60,6 +69,10 @@ class SplitTestBase(ModuleStoreTestCase):
         )
 
     def _problem(self, parent, group):
+        """
+        Returns a problem component with parent ``parent``
+        that is intended to be displayed to group ``group``.
+        """
         return ItemFactory.create(
             parent_location=parent.location,
             category="problem",
@@ -68,6 +81,10 @@ class SplitTestBase(ModuleStoreTestCase):
         )
 
     def _html(self, parent, group):
+        """
+        Returns an html component with parent ``parent``
+        that is intended to be displayed to group ``group``.
+        """
         return ItemFactory.create(
             parent_location=parent.location,
             category="html",
@@ -82,21 +99,23 @@ class SplitTestBase(ModuleStoreTestCase):
         self._check_split_test(1)
 
     def _check_split_test(self, user_tag):
-        tag_factory = UserCourseTagFactory(
+        """Checks that the right compentents are rendered for user with ``user_tag``"""
+        # This explicitly sets the user_tag for self.student to ``user_tag``
+        UserCourseTagFactory(
             user=self.student,
             course_id=self.course.id,
             key='xblock.partition_service.partition_{0}'.format(self.partition.id),
             value=str(user_tag)
         )
 
-        resp = self.client.get(reverse('courseware_section',
-                                       kwargs={'course_id': self.course.id,
-                                               'chapter': self.chapter.url_name,
-                                               'section': self.sequential.url_name}
+        resp = self.client.get(reverse(
+            'courseware_section',
+            kwargs={'course_id': self.course.id,
+                    'chapter': self.chapter.url_name,
+                    'section': self.sequential.url_name}
         ))
 
         content = resp.content
-        print content
 
         # Assert we see the proper icon in the top display
         self.assertIn('<a class="{} inactive progress-0"'.format(self.ICON_CLASSES[user_tag]), content)
@@ -118,7 +137,7 @@ class TestVertSplitTestVert(SplitTestBase):
     """
     __test__ = True
 
-    COURSE_NUMBER='vert-split-vert'
+    COURSE_NUMBER = 'vert-split-vert'
 
     ICON_CLASSES = [
         'seq_problem',
@@ -141,6 +160,8 @@ class TestVertSplitTestVert(SplitTestBase):
     ]
 
     def setUp(self):
+        # We define problem compenents that we need but don't explicitly call elsewhere.
+        # pylint: disable=unused-variable
         super(TestVertSplitTestVert, self).setUp()
 
         # vert <- split_test
@@ -151,6 +172,7 @@ class TestVertSplitTestVert(SplitTestBase):
             category="vertical",
             display_name="Split test vertical",
         )
+        # pylint: disable=protected-access
         c0_url = self.course.location._replace(category="vertical", name="split_test_cond0")
         c1_url = self.course.location._replace(category="vertical", name="split_test_cond1")
 
@@ -210,10 +232,13 @@ class TestSplitTestVert(SplitTestBase):
     ]
 
     def setUp(self):
+        # We define problem compenents that we need but don't explicitly call elsewhere.
+        # pylint: disable=unused-variable
         super(TestSplitTestVert, self).setUp()
 
         # split_test cond 0 = vert <- {video, problem}
         # split_test cond 1 = vert <- {video, html}
+        # pylint: disable=protected-access
         c0_url = self.course.location._replace(category="vertical", name="split_test_cond0")
         c1_url = self.course.location._replace(category="vertical", name="split_test_cond1")