From 484a4f2b662eb6f9b9d9a9216ccd393dc9577e23 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Wed, 14 Oct 2015 09:10:15 -0400 Subject: [PATCH 1/4] Make cohort management tests multiprocess-friendly. --- .../discussion/test_cohort_management.py | 68 +++++++++++++++---- 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 442707f042..5ab2c2ef98 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -5,6 +5,7 @@ End-to-end tests related to the cohort management on the LMS Instructor Dashboar from datetime import datetime +from path import path from pytz import UTC, utc from bok_choy.promise import EmptyPromise from nose.plugins.attrib import attr @@ -16,6 +17,8 @@ from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.instructor_dashboard import InstructorDashboardPage, DataDownloadPage from ...pages.studio.settings_group_configurations import GroupConfigurationsPage +import csv +import os import uuid @@ -39,23 +42,27 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.manual_cohort_id = self.add_manual_cohort(self.course_fixture, self.manual_cohort_name) # create a non-instructor who will be registered for the course and in the manual cohort. - self.student_name = "student_user" + self.student_name = "student_" + str(uuid.uuid4().hex)[:12] + self.student_email = self.student_name + "@example.com" self.student_id = AutoAuthPage( - self.browser, username=self.student_name, email="student_user@example.com", + self.browser, username=self.student_name, email=self.student_email, course_id=self.course_id, staff=False ).visit().get_user_id() self.add_user_to_cohort(self.course_fixture, self.student_name, self.manual_cohort_id) # create a second student user + self.other_student_name = "other_" + str(uuid.uuid4().hex)[:12] + self.other_student_email = self.other_student_name + "@example.com" self.other_student_id = AutoAuthPage( - self.browser, username="other_student_user", email="other_student_user@example.com", + self.browser, username=self.other_student_name, email=self.other_student_email, course_id=self.course_id, staff=False ).visit().get_user_id() # login as an instructor - self.instructor_name = "instructor_user" + self.instructor_name = "instructor_" + str(uuid.uuid4().hex)[:12] + self.instructor_email = self.instructor_name + "@example.com" self.instructor_id = AutoAuthPage( - self.browser, username=self.instructor_name, email="instructor_user@example.com", + self.browser, username=self.instructor_name, email=self.instructor_email, course_id=self.course_id, staff=True ).visit().get_user_id() @@ -64,6 +71,11 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.instructor_dashboard_page.visit() self.cohort_management_page = self.instructor_dashboard_page.select_cohort_management() + test_dir = path(__file__).abspath().dirname().dirname().dirname().dirname() + self.files_path = test_dir + '/data/uploads/' + + test_dir2 = self.instructor_dashboard_page.get_asset_path('.') + def verify_cohort_description(self, cohort_name, expected_description): """ Selects the cohort with the given name and verifies the expected description is presented. @@ -310,6 +322,16 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.cohort_management_page.get_cohort_associated_assignment_type() ) + def _create_csv_file(self, filename, csv_text_as_lists): + import csv + filename = self.instructor_dashboard_page.get_asset_path(filename) + # filename = self.files_path + filename + with open(filename, 'w+') as csv_file: + writer = csv.writer(csv_file, quoting=csv.QUOTE_ALL) + for line in csv_text_as_lists: + writer.writerow(line) + self.addCleanup(os.remove, filename) + def test_add_new_cohort(self): """ Scenario: A new manual cohort can be created, and a student assigned to it. @@ -471,9 +493,16 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin Then I can download a file with results And appropriate events have been emitted """ - # cohort_users_both_columns.csv adds instructor_user to ManualCohort1 via username and - # student_user to AutoCohort1 via email - self._verify_csv_upload_acceptable_file("cohort_users_both_columns.csv") + csv_contents = [ + ['username','email','ignored_column','cohort'], + [self.instructor_name,'','June','ManualCohort1'], + ['',self.student_email,'Spring','AutoCohort1'], + [self.other_student_name,'','Fall','ManualCohort1'], + ] + filename = "cohort_csv_both_columns_1.csv" + self._create_csv_file(filename, csv_contents) + self._verify_csv_upload_acceptable_file(filename) + # self._verify_csv_upload_acceptable_file("cohort_users_both_columns.csv") def test_cohort_by_csv_only_email(self): """ @@ -485,8 +514,15 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin Then I can download a file with results And appropriate events have been emitted """ - # cohort_users_only_email.csv adds instructor_user to ManualCohort1 and student_user to AutoCohort1 via email - self._verify_csv_upload_acceptable_file("cohort_users_only_email.csv") + csv_contents = [ + ['email', 'cohort'], + [self.instructor_email, 'ManualCohort1'], + [self.student_email, 'AutoCohort1'], + [self.other_student_email, 'ManualCohort1'], + ] + filename = "cohort_csv_emails_only.csv" + self._create_csv_file(filename, csv_contents) + self._verify_csv_upload_acceptable_file(filename) def test_cohort_by_csv_only_username(self): """ @@ -498,9 +534,15 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin Then I can download a file with results And appropriate events have been emitted """ - # cohort_users_only_username.csv adds instructor_user to ManualCohort1 and - # student_user to AutoCohort1 via username - self._verify_csv_upload_acceptable_file("cohort_users_only_username.csv") + csv_contents = [ + ['username', 'cohort'], + [self.instructor_name,'ManualCohort1'], + [self.student_name, 'AutoCohort1'], + [self.other_student_name, 'ManualCohort1'], + ] + filename = "cohort_users_only_username1.csv" + self._create_csv_file(filename, csv_contents) + self._verify_csv_upload_acceptable_file(filename) def _verify_csv_upload_acceptable_file(self, filename): """ From bed5d3e8dfa533952541bfd8b8a9ac872d9efa95 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Sun, 22 Nov 2015 09:35:53 -0500 Subject: [PATCH 2/4] Remove unnecessary files, use generic file for unrelated test. --- .../discussion/test_cohort_management.py | 25 +++++++++---------- .../tests/lms/test_learner_profile.py | 2 +- .../uploads/cohort_users_both_columns.csv | 4 --- .../data/uploads/cohort_users_only_email.csv | 5 ---- .../uploads/cohort_users_only_username.csv | 4 --- common/test/data/uploads/generic_csv.csv | 7 ++++++ 6 files changed, 20 insertions(+), 27 deletions(-) delete mode 100644 common/test/data/uploads/cohort_users_both_columns.csv delete mode 100644 common/test/data/uploads/cohort_users_only_email.csv delete mode 100644 common/test/data/uploads/cohort_users_only_username.csv create mode 100644 common/test/data/uploads/generic_csv.csv diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 5ab2c2ef98..063a9f2158 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -5,7 +5,6 @@ End-to-end tests related to the cohort management on the LMS Instructor Dashboar from datetime import datetime -from path import path from pytz import UTC, utc from bok_choy.promise import EmptyPromise from nose.plugins.attrib import attr @@ -71,11 +70,6 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.instructor_dashboard_page.visit() self.cohort_management_page = self.instructor_dashboard_page.select_cohort_management() - test_dir = path(__file__).abspath().dirname().dirname().dirname().dirname() - self.files_path = test_dir + '/data/uploads/' - - test_dir2 = self.instructor_dashboard_page.get_asset_path('.') - def verify_cohort_description(self, cohort_name, expected_description): """ Selects the cohort with the given name and verifies the expected description is presented. @@ -323,9 +317,14 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin ) def _create_csv_file(self, filename, csv_text_as_lists): - import csv + """ + Create a csv file with the provided list of lists. + + :param filename: this is the name that will be used for the csv file. Its location will + be under the test upload data directory + :param csv_text_as_lists: provide the contents of the csv file int he form of a list of lists + """ filename = self.instructor_dashboard_page.get_asset_path(filename) - # filename = self.files_path + filename with open(filename, 'w+') as csv_file: writer = csv.writer(csv_file, quoting=csv.QUOTE_ALL) for line in csv_text_as_lists: @@ -494,10 +493,10 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin And appropriate events have been emitted """ csv_contents = [ - ['username','email','ignored_column','cohort'], - [self.instructor_name,'','June','ManualCohort1'], - ['',self.student_email,'Spring','AutoCohort1'], - [self.other_student_name,'','Fall','ManualCohort1'], + ['username', 'email', 'ignored_column', 'cohort'], + [self.instructor_name, '', 'June', 'ManualCohort1'], + ['', self.student_email, 'Spring', 'AutoCohort1'], + [self.other_student_name, '', 'Fall', 'ManualCohort1'], ] filename = "cohort_csv_both_columns_1.csv" self._create_csv_file(filename, csv_contents) @@ -536,7 +535,7 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin """ csv_contents = [ ['username', 'cohort'], - [self.instructor_name,'ManualCohort1'], + [self.instructor_name, 'ManualCohort1'], [self.student_name, 'AutoCohort1'], [self.other_student_name, 'ManualCohort1'], ] diff --git a/common/test/acceptance/tests/lms/test_learner_profile.py b/common/test/acceptance/tests/lms/test_learner_profile.py index d32a489451..1b27c1b8b6 100644 --- a/common/test/acceptance/tests/lms/test_learner_profile.py +++ b/common/test/acceptance/tests/lms/test_learner_profile.py @@ -604,7 +604,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): self.assert_default_image_has_public_access(profile_page) - profile_page.upload_file(filename='cohort_users_only_username.csv') + profile_page.upload_file(filename='generic_csv.csv') self.assertEqual( profile_page.profile_image_message, "The file must be one of the following types: .gif, .png, .jpeg, .jpg." diff --git a/common/test/data/uploads/cohort_users_both_columns.csv b/common/test/data/uploads/cohort_users_both_columns.csv deleted file mode 100644 index ee13b56c7c..0000000000 --- a/common/test/data/uploads/cohort_users_both_columns.csv +++ /dev/null @@ -1,4 +0,0 @@ -username,email,ignored_column,cohort -instructor_user,,June,ManualCohort1 -,student_user@example.com,Spring,AutoCohort1 -other_student_user,,Fall,ManualCohort1 diff --git a/common/test/data/uploads/cohort_users_only_email.csv b/common/test/data/uploads/cohort_users_only_email.csv deleted file mode 100644 index 7fb6a85400..0000000000 --- a/common/test/data/uploads/cohort_users_only_email.csv +++ /dev/null @@ -1,5 +0,0 @@ -email,cohort -instructor_user@example.com,ManualCohort1 -student_user@example.com,AutoCohort1 -other_student_user@example.com,ManualCohort1 - diff --git a/common/test/data/uploads/cohort_users_only_username.csv b/common/test/data/uploads/cohort_users_only_username.csv deleted file mode 100644 index f33b77a0a2..0000000000 --- a/common/test/data/uploads/cohort_users_only_username.csv +++ /dev/null @@ -1,4 +0,0 @@ -username,cohort -instructor_user,ManualCohort1 -student_user,AutoCohort1 -other_student_user,ManualCohort1 diff --git a/common/test/data/uploads/generic_csv.csv b/common/test/data/uploads/generic_csv.csv new file mode 100644 index 0000000000..bad83f6563 --- /dev/null +++ b/common/test/data/uploads/generic_csv.csv @@ -0,0 +1,7 @@ +column_1,column_2 +foo,bar +foo,baz +hello,there +file_size,must_be_100_bytes +the story,was +very very very very very,interesting From e512a50649ce5f6df1a39283e1311c07d6d90f0a Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Mon, 23 Nov 2015 16:56:42 -0500 Subject: [PATCH 3/4] Use helper method for generating unique user identities. --- .../tests/discussion/test_cohort_management.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 063a9f2158..6c5778b491 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -41,8 +41,7 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.manual_cohort_id = self.add_manual_cohort(self.course_fixture, self.manual_cohort_name) # create a non-instructor who will be registered for the course and in the manual cohort. - self.student_name = "student_" + str(uuid.uuid4().hex)[:12] - self.student_email = self.student_name + "@example.com" + self.student_name, self.student_email = self._generate_unique_user_data() self.student_id = AutoAuthPage( self.browser, username=self.student_name, email=self.student_email, course_id=self.course_id, staff=False @@ -50,16 +49,14 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self.add_user_to_cohort(self.course_fixture, self.student_name, self.manual_cohort_id) # create a second student user - self.other_student_name = "other_" + str(uuid.uuid4().hex)[:12] - self.other_student_email = self.other_student_name + "@example.com" + self.other_student_name, self.other_student_email = self._generate_unique_user_data() self.other_student_id = AutoAuthPage( self.browser, username=self.other_student_name, email=self.other_student_email, course_id=self.course_id, staff=False ).visit().get_user_id() # login as an instructor - self.instructor_name = "instructor_" + str(uuid.uuid4().hex)[:12] - self.instructor_email = self.instructor_name + "@example.com" + self.instructor_name, self.instructor_email = self._generate_unique_user_data() self.instructor_id = AutoAuthPage( self.browser, username=self.instructor_name, email=self.instructor_email, course_id=self.course_id, staff=True @@ -331,6 +328,12 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin writer.writerow(line) self.addCleanup(os.remove, filename) + def _generate_unique_user_data(self): + unique_username = 'user' + str(uuid.uuid4().hex)[:12] + unique_email = unique_username + "@example.com" + return unique_username, unique_email + + def test_add_new_cohort(self): """ Scenario: A new manual cohort can be created, and a student assigned to it. From 2b61cfb1a26bc4a87fbdce48e4aaf97d575f6e4d Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Tue, 24 Nov 2015 16:57:01 -0500 Subject: [PATCH 4/4] Use unicodecsv. Add scenario for testing unicode cohort. Verifying the events is a TODO because the verification method will need to be refactored to handle an additional cohort that's not included in setUp. (Or refactor the setup, or refactor the unicode test, etc.) --- .../discussion/test_cohort_management.py | 117 +++++++++++------- 1 file changed, 73 insertions(+), 44 deletions(-) diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 6c5778b491..8b61f8e3f2 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -16,8 +16,8 @@ from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.instructor_dashboard import InstructorDashboardPage, DataDownloadPage from ...pages.studio.settings_group_configurations import GroupConfigurationsPage -import csv import os +import unicodecsv import uuid @@ -323,17 +323,19 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin """ filename = self.instructor_dashboard_page.get_asset_path(filename) with open(filename, 'w+') as csv_file: - writer = csv.writer(csv_file, quoting=csv.QUOTE_ALL) + writer = unicodecsv.writer(csv_file) for line in csv_text_as_lists: writer.writerow(line) self.addCleanup(os.remove, filename) def _generate_unique_user_data(self): + """ + Produce unique username and e-mail. + """ unique_username = 'user' + str(uuid.uuid4().hex)[:12] unique_email = unique_username + "@example.com" return unique_username, unique_email - def test_add_new_cohort(self): """ Scenario: A new manual cohort can be created, and a student assigned to it. @@ -504,7 +506,6 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin filename = "cohort_csv_both_columns_1.csv" self._create_csv_file(filename, csv_contents) self._verify_csv_upload_acceptable_file(filename) - # self._verify_csv_upload_acceptable_file("cohort_users_both_columns.csv") def test_cohort_by_csv_only_email(self): """ @@ -546,9 +547,36 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin self._create_csv_file(filename, csv_contents) self._verify_csv_upload_acceptable_file(filename) - def _verify_csv_upload_acceptable_file(self, filename): + def test_cohort_by_csv_unicode(self): + """ + Scenario: the instructor can upload a file with user and cohort assignments, using both emails and usernames. + + Given I have a course with two cohorts defined + And I add another cohort with a unicode name + When I go to the cohort management section of the instructor dashboard + I can upload a CSV file with assignments of users to the unicode cohort via both usernames and emails + Then I can download a file with results + + TODO: refactor events verification to handle this scenario. Events verification assumes movements + between other cohorts (manual and auto). + """ + unicode_hello_in_korean = u'안녕하세요' + self._verify_cohort_settings(cohort_name=unicode_hello_in_korean, assignment_type=None) + csv_contents = [ + ['username', 'email', 'cohort'], + [self.instructor_name, '', unicode_hello_in_korean], + ['', self.student_email, unicode_hello_in_korean], + [self.other_student_name, '', unicode_hello_in_korean] + ] + filename = "cohort_unicode_name.csv" + self._create_csv_file(filename, csv_contents) + self._verify_csv_upload_acceptable_file(filename, skip_events=True) + + def _verify_csv_upload_acceptable_file(self, filename, skip_events=None): """ Helper method to verify cohort assignments after a successful CSV upload. + + When skip_events is specified, no assertions are made on events. """ start_time = datetime.now(UTC) self.cohort_management_page.upload_cohort_file(filename) @@ -556,45 +584,46 @@ class CohortConfigurationTest(EventsTestMixin, UniqueCourseTest, CohortTestMixin "Your file '{}' has been uploaded. Allow a few minutes for processing.".format(filename) ) - # student_user is moved from manual cohort to auto cohort - self.assertEqual( - self.event_collection.find({ - "name": "edx.cohort.user_added", - "time": {"$gt": start_time}, - "event.user_id": {"$in": [int(self.student_id)]}, - "event.cohort_name": self.auto_cohort_name, - }).count(), - 1 - ) - self.assertEqual( - self.event_collection.find({ - "name": "edx.cohort.user_removed", - "time": {"$gt": start_time}, - "event.user_id": int(self.student_id), - "event.cohort_name": self.manual_cohort_name, - }).count(), - 1 - ) - # instructor_user (previously unassigned) is added to manual cohort - self.assertEqual( - self.event_collection.find({ - "name": "edx.cohort.user_added", - "time": {"$gt": start_time}, - "event.user_id": {"$in": [int(self.instructor_id)]}, - "event.cohort_name": self.manual_cohort_name, - }).count(), - 1 - ) - # other_student_user (previously unassigned) is added to manual cohort - self.assertEqual( - self.event_collection.find({ - "name": "edx.cohort.user_added", - "time": {"$gt": start_time}, - "event.user_id": {"$in": [int(self.other_student_id)]}, - "event.cohort_name": self.manual_cohort_name, - }).count(), - 1 - ) + if not skip_events: + # student_user is moved from manual cohort to auto cohort + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.student_id)]}, + "event.cohort_name": self.auto_cohort_name, + }).count(), + 1 + ) + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_removed", + "time": {"$gt": start_time}, + "event.user_id": int(self.student_id), + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + # instructor_user (previously unassigned) is added to manual cohort + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.instructor_id)]}, + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) + # other_student_user (previously unassigned) is added to manual cohort + self.assertEqual( + self.event_collection.find({ + "name": "edx.cohort.user_added", + "time": {"$gt": start_time}, + "event.user_id": {"$in": [int(self.other_student_id)]}, + "event.cohort_name": self.manual_cohort_name, + }).count(), + 1 + ) # Verify the results can be downloaded. data_download = self.instructor_dashboard_page.select_data_download()