From 81d17c516ec7e47b7a6b0210ec43de8e34048b7b Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Sat, 14 Mar 2015 21:39:45 -0400 Subject: [PATCH] Fix or remove tearDown methods that don't use super. Update edx-lint to the version that checks if tearDown uses super. Convert a number of tearDown methods into addCleanup. Remove some unneeded tearDown methods: no need to call patch.stopall if none of them were started with patch.start. --- .../commands/tests/test_export_all_courses.py | 5 +--- .../tests/test_export_convert_format.py | 5 +--- .../tests/test_transcripts_utils.py | 6 ++--- .../views/tests/test_course_index.py | 5 ++-- .../views/tests/test_import_export.py | 4 +--- .../lib/xmodule/xmodule/tests/test_export.py | 5 +--- .../lib/xmodule/xmodule/tests/test_video.py | 1 + lms/djangoapps/branding/tests/test_models.py | 1 + .../bulk_email/tests/test_course_optout.py | 7 ------ lms/djangoapps/bulk_email/tests/test_email.py | 6 ----- .../bulk_email/tests/test_err_handling.py | 3 --- lms/djangoapps/bulk_email/tests/test_forms.py | 6 ----- .../courseware/tests/test_group_access.py | 1 + .../courseware/tests/test_video_handlers.py | 1 + .../dashboard/tests/test_support.py | 1 + lms/djangoapps/django_comment_client/tests.py | 13 ++++------- .../mock_cs_server/test_mock_cs_server.py | 5 +--- lms/djangoapps/instructor/tests/test_api.py | 23 +------------------ .../instructor/tests/test_ecommerce.py | 7 ------ lms/djangoapps/instructor/tests/test_email.py | 6 ----- .../shoppingcart/tests/test_models.py | 1 + requirements/edx/github.txt | 2 +- 22 files changed, 22 insertions(+), 92 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py b/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py index 79282ac32a..5013bce8cd 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py @@ -21,6 +21,7 @@ class ExportAllCourses(ModuleStoreTestCase): super(ExportAllCourses, self).setUp() self.store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) self.temp_dir = mkdtemp() + self.addCleanup(shutil.rmtree, self.temp_dir) self.first_course = CourseFactory.create( org="test", course="course1", display_name="run1", default_store=ModuleStoreEnum.Type.mongo ) @@ -47,7 +48,3 @@ class ExportAllCourses(ModuleStoreTestCase): self.assertEqual(len(courses), 2) self.assertEqual(len(failed_export_courses), 1) self.assertEqual(failed_export_courses[0], unicode(second_course_id)) - - def tearDown(self): - """ Common cleanup. """ - shutil.rmtree(self.temp_dir) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py b/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py index b457a22465..fd83d58f89 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py @@ -19,16 +19,13 @@ class ConvertExportFormat(TestCase): super(ConvertExportFormat, self).setUp() self.temp_dir = mkdtemp() + self.addCleanup(shutil.rmtree, self.temp_dir) self.data_dir = path(__file__).realpath().parent / 'data' self.version0 = self.data_dir / "Version0_drafts.tar.gz" self.version1 = self.data_dir / "Version1_drafts.tar.gz" self.command = Command() - def tearDown(self): - """ Common cleanup. """ - shutil.rmtree(self.temp_dir) - def test_no_args(self): """ Test error condition of no arguments. """ errstring = "export requires two arguments" diff --git a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py b/cms/djangoapps/contentstore/tests/test_transcripts_utils.py index 65e86ddc4f..ef762471d1 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py +++ b/cms/djangoapps/contentstore/tests/test_transcripts_utils.py @@ -112,8 +112,9 @@ class TestSaveSubsToStore(ModuleStoreTestCase): self.subs_id = str(uuid4()) filename = 'subs_{0}.srt.sjson'.format(self.subs_id) self.content_location = StaticContent.compute_location(self.course.id, filename) + self.addCleanup(self.clear_subs_content) - # incorrect subs + # incorrect subs self.unjsonable_subs = set([1]) # set can't be serialized self.unjsonable_subs_id = str(uuid4()) @@ -150,9 +151,6 @@ class TestSaveSubsToStore(ModuleStoreTestCase): with self.assertRaises(NotFoundError): contentstore().find(self.content_location_unjsonable) - def tearDown(self): - self.clear_subs_content() - @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class TestDownloadYoutubeSubs(ModuleStoreTestCase): diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 7304ab73d9..f44cfe49f8 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -383,6 +383,8 @@ class TestCourseReIndex(CourseTestCase): with open(self.TEST_INDEX_FILENAME, "w+") as index_file: json.dump({}, index_file) + self.addCleanup(os.remove, self.TEST_INDEX_FILENAME) + def test_reindex_course(self): """ Verify that course gets reindexed. @@ -666,6 +668,3 @@ class TestCourseReIndex(CourseTestCase): # Start manual reindex and check error in response with self.assertRaises(SearchIndexingError): CoursewareSearchIndexer.do_course_reindex(modulestore(), self.course.id) - - def tearDown(self): - os.remove(self.TEST_INDEX_FILENAME) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index ca369e9224..186525b1d6 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -43,6 +43,7 @@ class ImportTestCase(CourseTestCase): super(ImportTestCase, self).setUp() self.url = reverse_course_url('import_handler', self.course.id) self.content_dir = path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.content_dir) def touch(name): """ Equivalent to shell's 'touch'""" @@ -74,9 +75,6 @@ class ImportTestCase(CourseTestCase): self.unsafe_common_dir = path(tempfile.mkdtemp(dir=self.content_dir)) - def tearDown(self): - shutil.rmtree(self.content_dir) - def test_no_coursexml(self): """ Check that the response for a tar.gz import without a course.xml is diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index 893f0523b1..0f240e61a3 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -227,6 +227,7 @@ class ConvertExportFormat(unittest.TestCase): # Directory for expanding all the test archives self.temp_dir = mkdtemp() + self.addCleanup(shutil.rmtree, self.temp_dir) # Directory where new archive will be created self.result_dir = path(self.temp_dir) / uuid.uuid4().hex @@ -284,10 +285,6 @@ class ConvertExportFormat(unittest.TestCase): self._no_version = self._expand_archive('NoVersionNumber.tar.gz') return self._no_version - def tearDown(self): - """ Common cleanup. """ - shutil.rmtree(self.temp_dir) - def _expand_archive(self, name): """ Expand archive into a directory and return the directory. """ target = path(self.temp_dir) / uuid.uuid4().hex diff --git a/common/lib/xmodule/xmodule/tests/test_video.py b/common/lib/xmodule/xmodule/tests/test_video.py index 98969a7282..e6fe2ee505 100644 --- a/common/lib/xmodule/xmodule/tests/test_video.py +++ b/common/lib/xmodule/xmodule/tests/test_video.py @@ -642,6 +642,7 @@ class VideoDescriptorIndexingTestCase(unittest.TestCase): """ Overrides YOUTUBE and CONTENTSTORE settings """ + super(VideoDescriptorIndexingTestCase, self).setUp() self.youtube_setting = getattr(settings, "YOUTUBE", None) self.contentstore_setting = getattr(settings, "CONTENTSTORE", None) settings.YOUTUBE = { diff --git a/lms/djangoapps/branding/tests/test_models.py b/lms/djangoapps/branding/tests/test_models.py index 6df0a7059f..7d0876bdb0 100644 --- a/lms/djangoapps/branding/tests/test_models.py +++ b/lms/djangoapps/branding/tests/test_models.py @@ -11,6 +11,7 @@ class BrandingInfoConfigTest(TestCase): Test the BrandingInfoConfig model. """ def setUp(self): + super(BrandingInfoConfigTest, self).setUp() self.configuration_string = """{ "CN": { "url": "http://www.xuetangx.com", diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 61e7e24401..05f478aa84 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py @@ -18,7 +18,6 @@ from xmodule.modulestore.tests.factories import CourseFactory @patch('bulk_email.models.html_to_text', Mock(return_value='Mocking CourseEmail.text_message')) class TestOptoutCourseEmails(ModuleStoreTestCase): - """ Test that optouts are referenced in sending course email. """ @@ -42,12 +41,6 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): 'success': True, } - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - def navigate_to_email_view(self): """Navigate to the instructor dash's email view""" # Pull up email view on instructor dashboard diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 5f4e33c1c7..d825a93c44 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -84,12 +84,6 @@ class EmailSendFromDashboardTestCase(ModuleStoreTestCase): 'success': True, } - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - @patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) @patch('bulk_email.models.html_to_text', Mock(return_value='Mocking CourseEmail.text_message')) diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index dd01b24330..818eeb5fca 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -58,9 +58,6 @@ class TestEmailErrors(ModuleStoreTestCase): 'success': True, } - def tearDown(self): - patch.stopall() - @patch('bulk_email.tasks.get_connection', autospec=True) @patch('bulk_email.tasks.send_course_email.retry') def test_data_err_retry(self, retry, get_conn): diff --git a/lms/djangoapps/bulk_email/tests/test_forms.py b/lms/djangoapps/bulk_email/tests/test_forms.py index e9c0230ef6..863c4abe95 100644 --- a/lms/djangoapps/bulk_email/tests/test_forms.py +++ b/lms/djangoapps/bulk_email/tests/test_forms.py @@ -23,12 +23,6 @@ class CourseAuthorizationFormTest(ModuleStoreTestCase): course_title = u"ẗëṡẗ title ｲ乇丂ｲ ﾶ乇丂丂ﾑg乇 ｷo尺 ﾑﾚﾚ тэѕт мэѕѕаБэ" self.course = CourseFactory.create(display_name=course_title) - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - @patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) def test_authorize_mongo_course(self): # Initially course shouldn't be authorized diff --git a/lms/djangoapps/courseware/tests/test_group_access.py b/lms/djangoapps/courseware/tests/test_group_access.py index 28793b1929..02174634e8 100644 --- a/lms/djangoapps/courseware/tests/test_group_access.py +++ b/lms/djangoapps/courseware/tests/test_group_access.py @@ -176,6 +176,7 @@ class GroupAccessTestCase(ModuleStoreTestCase): side-effects in other tests. """ UserPartition.scheme_extensions = None + super(GroupAccessTestCase, self).tearDown() def check_access(self, user, block_location, is_accessible): """ diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 4d80952848..581418f7df 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -154,6 +154,7 @@ class TestVideo(BaseTestXmodule): def tearDown(self): _clear_assets(self.item_descriptor.location) + super(TestVideo, self).tearDown() class TestTranscriptAvailableTranslationsDispatch(TestVideo): diff --git a/lms/djangoapps/dashboard/tests/test_support.py b/lms/djangoapps/dashboard/tests/test_support.py index 544cf40608..408867fbc7 100644 --- a/lms/djangoapps/dashboard/tests/test_support.py +++ b/lms/djangoapps/dashboard/tests/test_support.py @@ -45,6 +45,7 @@ class RefundTests(ModuleStoreTestCase): def tearDown(self): self.course_mode.delete() Order.objects.filter(user=self.student).delete() + super(RefundTests, self).tearDown() def _enroll(self, purchase=True): # pylint: disable=missing-docstring diff --git a/lms/djangoapps/django_comment_client/tests.py b/lms/djangoapps/django_comment_client/tests.py index 6edbfd58bb..e4df90b9cb 100644 --- a/lms/djangoapps/django_comment_client/tests.py +++ b/lms/djangoapps/django_comment_client/tests.py @@ -31,15 +31,12 @@ class PermissionsTestCase(TestCase): self.moderator.is_staff = True self.moderator.save() self.student_enrollment = CourseEnrollment.enroll(self.student, self.course_id) + self.addCleanup(self.student_enrollment.delete) self.moderator_enrollment = CourseEnrollment.enroll(self.moderator, self.course_id) - - def tearDown(self): - self.student_enrollment.delete() - self.moderator_enrollment.delete() - -# Do we need to have this? We shouldn't be deleting students, ever -# self.student.delete() -# self.moderator.delete() + self.addCleanup(self.moderator_enrollment.delete) + # Do we need to have this in a cleanup? We shouldn't be deleting students, ever. + # self.student.delete() + # self.moderator.delete() def testDefaultRoles(self): self.assertTrue(self.student_role in self.student.roles.all()) diff --git a/lms/djangoapps/django_comment_client/tests/mock_cs_server/test_mock_cs_server.py b/lms/djangoapps/django_comment_client/tests/mock_cs_server/test_mock_cs_server.py index 7518bd6895..1aea668329 100644 --- a/lms/djangoapps/django_comment_client/tests/mock_cs_server/test_mock_cs_server.py +++ b/lms/djangoapps/django_comment_client/tests/mock_cs_server/test_mock_cs_server.py @@ -29,16 +29,13 @@ class MockCommentServiceServerTest(unittest.TestCase): self.expected_response = {'username': 'user100', 'external_id': '4'} self.server = MockCommentServiceServer(port_num=server_port, response=self.expected_response) + self.addCleanup(self.server.shutdown) # Start the server in a separate daemon thread server_thread = threading.Thread(target=self.server.serve_forever) server_thread.daemon = True server_thread.start() - def tearDown(self): - # Stop the server, freeing up the port - self.server.shutdown() - def test_new_user_request(self): """ Test the mock comment service using an example diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 638db67d17..a95de64697 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -602,12 +602,6 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): # (comment because pylint C0103(invalid-name)) # self.maxDiff = None - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - def test_missing_params(self): """ Test missing all query parameters. """ url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id.to_deprecated_string()}) @@ -2725,12 +2719,6 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): self.tasks = [self.FakeTask(mock_factory.mock_get_task_completion_info) for _ in xrange(7)] self.tasks[-1].make_invalid_output() - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - @patch.object(instructor_task.api, 'get_running_instructor_tasks') def test_list_instructor_tasks_running(self, act): """ Test list of all running tasks. """ @@ -2830,12 +2818,6 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas self.emails = {} self.emails_info = {} - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - def setup_fake_email_info(self, num_emails, with_failures=False): """ Initialize the specified number of fake emails """ for email_id in range(num_emails): @@ -3783,10 +3765,7 @@ class TestBulkCohorting(ModuleStoreTestCase): self.staff_user = StaffFactory(course_key=self.course.id) self.non_staff_user = UserFactory.create() self.tempdir = tempfile.mkdtemp() - - def tearDown(self): - if os.path.exists(self.tempdir): - shutil.rmtree(self.tempdir) + self.addCleanup(shutil.rmtree, self.tempdir) def call_add_users_to_cohorts(self, csv_data, suffix='.csv', method='POST'): """ diff --git a/lms/djangoapps/instructor/tests/test_ecommerce.py b/lms/djangoapps/instructor/tests/test_ecommerce.py index 3fe841a270..fc14454324 100644 --- a/lms/djangoapps/instructor/tests/test_ecommerce.py +++ b/lms/djangoapps/instructor/tests/test_ecommerce.py @@ -4,7 +4,6 @@ Unit tests for Ecommerce feature flag in new instructor dashboard. import datetime -from mock import patch import pytz from django.core.urlresolvers import reverse @@ -38,12 +37,6 @@ class TestECommerceDashboardViews(ModuleStoreTestCase): self.e_commerce_link = 'E-Commerce' CourseFinanceAdminRole(self.course.id).add_users(self.instructor) - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - def test_pass_e_commerce_tab_in_instructor_dashboard(self): """ Test Pass E-commerce Tab is in the Instructor Dashboard diff --git a/lms/djangoapps/instructor/tests/test_email.py b/lms/djangoapps/instructor/tests/test_email.py index 803cbf1ead..2f3faac5de 100644 --- a/lms/djangoapps/instructor/tests/test_email.py +++ b/lms/djangoapps/instructor/tests/test_email.py @@ -33,12 +33,6 @@ class TestNewInstructorDashboardEmailViewMongoBacked(ModuleStoreTestCase): # URL for email view self.email_link = 'Email' - def tearDown(self): - """ - Undo all patches. - """ - patch.stopall() - # In order for bulk email to work, we must have both the ENABLE_INSTRUCTOR_EMAIL_FLAG # set to True and for the course to be Mongo-backed. # The flag is enabled and the course is Mongo-backed (should work) diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index 7fe3044fb0..b3581e0832 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/lms/djangoapps/shoppingcart/tests/test_models.py @@ -926,6 +926,7 @@ class InvoiceHistoryTest(TestCase): } def setUp(self): + super(InvoiceHistoryTest, self).setUp() invoice_data = copy.copy(self.INVOICE_INFO) invoice_data.update(self.CONTACT_INFO) self.invoice = Invoice.objects.create(total_amount="123.45", **invoice_data) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 83944b11c1..2951d34541 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -39,7 +39,7 @@ git+https://github.com/mitocw/django-cas.git@60a5b8e5a62e63e0d5d224a87f0b489201a -e git+https://github.com/pmitros/RecommenderXBlock.git@9b07e807c89ba5761827d0387177f71aa57ef056#egg=recommender-xblock -e git+https://github.com/edx/edx-milestones.git@547f2250ee49e73ce8d7ff4e78ecf1b049892510#egg=edx-milestones -e git+https://github.com/edx/edx-search.git@264bb3317f98e9cb22b932aa11b89d0651fd741c#egg=edx-search -git+https://github.com/edx/edx-lint.git@c592557ecee8f2f1ef14ad984e1353f41aba0b47#egg=edx_lint==0.2 +git+https://github.com/edx/edx-lint.git@8bf82a32ecb8598c415413df66f5232ab8d974e9#egg=edx_lint==0.2.1 -e git+https://github.com/edx/xblock-utils.git@17e247d66fabb53f0453515b093a030ee345a1b0#egg=xblock-utils -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive