From 1df35507a330f35319aa74f1532d4bb28e9eee72 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 25 Mar 2013 14:13:47 -0400 Subject: [PATCH 1/7] Changed 'in' to '==' to fix bug #258 --- common/lib/capa/capa/templates/choicegroup.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/capa/capa/templates/choicegroup.html b/common/lib/capa/capa/templates/choicegroup.html index e1ff40b6a1..c7840cfd4e 100644 --- a/common/lib/capa/capa/templates/choicegroup.html +++ b/common/lib/capa/capa/templates/choicegroup.html @@ -17,7 +17,7 @@ % for choice_id, choice_description in choices: From 7dcb1bf7c64f7c9ea2de847c09be3c63e3a6da12 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 16:09:13 -0400 Subject: [PATCH 3/7] it appears we are taking one too many round trips to do when pre-fetching children. This can be very expensive as the tree gets wider the deeper we go. For example, in courseware we want depth=2 (course, chapter, sequential). But looking at log output we were also getting verticals, which there can be a lot of. This should cut down on the total data we are grabbing from the DB. --- common/lib/xmodule/xmodule/modulestore/mongo.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index f6fa98fc28..b76251bb99 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -366,6 +366,9 @@ class MongoModuleStore(ModuleStoreBase): children.extend(item.get('definition', {}).get('children', [])) data[Location(item['location'])] = item + if depth == 0: + break; + # Load all children by id. See # http://www.mongodb.org/display/DOCS/Advanced+Queries#AdvancedQueries-%24or # for or-query syntax From c660229b25f4ef1f9b922c8b325e60cbcb35bcb3 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 25 Mar 2013 16:37:59 -0400 Subject: [PATCH 4/7] Added checking for problem answer state after a problem is checked --- .../courseware/features/problems.feature | 4 + .../courseware/features/problems.py | 92 ++++++++++++++++++- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/courseware/features/problems.feature b/lms/djangoapps/courseware/features/problems.feature index efeb338c45..dc8495af60 100644 --- a/lms/djangoapps/courseware/features/problems.feature +++ b/lms/djangoapps/courseware/features/problems.feature @@ -8,6 +8,7 @@ Feature: Answer problems And I am viewing a "" problem When I answer a "" problem "correctly" Then My "" answer is marked "correct" + And The "" problem displays a "correct" answer Examples: | ProblemType | @@ -25,6 +26,7 @@ Feature: Answer problems And I am viewing a "" problem When I answer a "" problem "incorrectly" Then My "" answer is marked "incorrect" + And The "" problem displays a "incorrect" answer Examples: | ProblemType | @@ -41,6 +43,7 @@ Feature: Answer problems Given I am viewing a "" problem When I check a problem Then My "" answer is marked "incorrect" + And The "" problem displays a "blank" answer Examples: | ProblemType | @@ -58,6 +61,7 @@ Feature: Answer problems And I answer a "" problem "ly" When I reset the problem Then My "" answer is marked "unanswered" + And The "" problem displays a "blank" answer Examples: | ProblemType | Correctness | diff --git a/lms/djangoapps/courseware/features/problems.py b/lms/djangoapps/courseware/features/problems.py index 6b2239c38b..36a0477988 100644 --- a/lms/djangoapps/courseware/features/problems.py +++ b/lms/djangoapps/courseware/features/problems.py @@ -2,8 +2,8 @@ from lettuce import world, step from lettuce.django import django_url import random import textwrap -import time -from common import i_am_registered_for_the_course, TEST_SECTION_NAME, section_location +from common import i_am_registered_for_the_course, \ + TEST_SECTION_NAME, section_location from capa.tests.response_xml_factory import OptionResponseXMLFactory, \ ChoiceResponseXMLFactory, MultipleChoiceResponseXMLFactory, \ StringResponseXMLFactory, NumericalResponseXMLFactory, \ @@ -26,7 +26,7 @@ PROBLEM_FACTORY_DICT = { 'kwargs': { 'question_text': 'The correct answer is Choice 3', 'choices': [False, False, True, False], - 'choice_names': ['choice_1', 'choice_2', 'choice_3', 'choice_4']}}, + 'choice_names': ['choice_0', 'choice_1', 'choice_2', 'choice_3']}}, 'checkbox': { 'factory': ChoiceResponseXMLFactory(), @@ -152,9 +152,9 @@ def answer_problem(step, problem_type, correctness): elif problem_type == "multiple choice": if correctness == 'correct': - inputfield('multiple choice', choice='choice_3').check() - else: inputfield('multiple choice', choice='choice_2').check() + else: + inputfield('multiple choice', choice='choice_1').check() elif problem_type == "checkbox": if correctness == 'correct': @@ -202,6 +202,65 @@ def answer_problem(step, problem_type, correctness): # Submit the problem check_problem(step) +@step(u'The "([^"]*)" problem displays a "([^"]*)" answer') +def assert_problem_has_answer(step, problem_type, answer_class): + ''' + Assert that the problem is displaying a particular answer. + These correspond to the same correct/incorrect + answers we set in answer_problem() + + We can also check that a problem has been left blank + by setting answer_class='blank' + ''' + assert answer_class in ['correct', 'incorrect', 'blank'] + + if problem_type == "drop down": + if answer_class == 'blank': + assert world.browser.is_element_not_present_by_css('option[selected="true"]') + else: + actual = world.browser.find_by_css('option[selected="true"]').value + expected = 'Option 2' if answer_class == 'correct' else 'Option 3' + assert actual == expected + + elif problem_type == "multiple choice": + if answer_class == 'correct': + assert_checked('multiple choice', ['choice_2']) + elif answer_class == 'incorrect': + assert_checked('multiple choice', ['choice_1']) + else: + assert_checked('multiple choice', []) + + elif problem_type == "checkbox": + if answer_class == 'correct': + assert_checked('checkbox', ['choice_0', 'choice_2']) + elif answer_class == 'incorrect': + assert_checked('checkbox', ['choice_3']) + else: + assert_checked('checkbox', []) + + elif problem_type == 'string': + if answer_class == 'blank': + expected = '' + else: + expected = 'correct string' if answer_class == 'correct' else 'incorrect' + + assert_textfield('string', expected) + + elif problem_type == 'formula': + if answer_class == 'blank': + expected = '' + else: + expected = "x^2+2*x+y" if answer_class == 'correct' else 'x^2' + + assert_textfield('formula', expected) + + else: + # The other response types use random data, + # which would be difficult to check + # We trade input value coverage in the other tests for + # input type coverage in this test. + pass + @step(u'I check a problem') def check_problem(step): @@ -274,6 +333,7 @@ def assert_answer_mark(step, problem_type, correctness): # Expect that we found the expected selector assert(has_expected) + def inputfield(problem_type, choice=None, input_num=1): """ Return the element for *problem_type*. For example, if problem_type is 'string', return @@ -289,8 +349,30 @@ def inputfield(problem_type, choice=None, input_num=1): base = "_choice_" if problem_type == "multiple choice" else "_" sel = sel + base + str(choice) + # If the input element doesn't exist, fail immediately assert(world.browser.is_element_present_by_css(sel, wait_time=4)) # Retrieve the input element return world.browser.find_by_css(sel) + +def assert_checked(problem_type, choices): + ''' + Assert that choice names given in *choices* are the only + ones checked. + + Works for both radio and checkbox problems + ''' + + all_choices = ['choice_0', 'choice_1', 'choice_2', 'choice_3'] + for ch in all_choices: + el = inputfield(problem_type, choice=ch) + + if ch in choices: + assert el.checked + else: + assert not el.checked + +def assert_textfield(problem_type, expected_text, input_num=1): + el = inputfield(problem_type, input_num=input_num) + assert el.value == expected_text From 37e7d68cef72174e33bdb4d2dab06b881ab69c9f Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 25 Mar 2013 16:46:31 -0400 Subject: [PATCH 5/7] pep8 and pylint fixes --- .../courseware/features/problems.py | 55 ++++++++++++------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/courseware/features/problems.py b/lms/djangoapps/courseware/features/problems.py index 36a0477988..d2d379a212 100644 --- a/lms/djangoapps/courseware/features/problems.py +++ b/lms/djangoapps/courseware/features/problems.py @@ -1,3 +1,8 @@ +''' +Steps for problem.feature lettuce tests +''' + + from lettuce import world, step from lettuce.django import django_url import random @@ -88,6 +93,9 @@ PROBLEM_FACTORY_DICT = { def add_problem_to_course(course, problem_type): + ''' + Add a problem to the course we have created using factories. + ''' assert(problem_type in PROBLEM_FACTORY_DICT) @@ -98,11 +106,12 @@ def add_problem_to_course(course, problem_type): # Create a problem item using our generated XML # We set rerandomize=always in the metadata so that the "Reset" button # will appear. - problem_item = world.ItemFactory.create(parent_location=section_location(course), - template="i4x://edx/templates/problem/Blank_Common_Problem", - display_name=str(problem_type), - data=problem_xml, - metadata={'rerandomize': 'always'}) + template_name = "i4x://edx/templates/problem/Blank_Common_Problem" + world.ItemFactory.create(parent_location=section_location(course), + template=template_name, + display_name=str(problem_type), + data=problem_xml, + metadata={'rerandomize': 'always'}) @step(u'I am viewing a "([^"]*)" problem') @@ -164,11 +173,13 @@ def answer_problem(step, problem_type, correctness): inputfield('checkbox', choice='choice_3').check() elif problem_type == 'string': - textvalue = 'correct string' if correctness == 'correct' else 'incorrect' + textvalue = 'correct string' if correctness == 'correct' \ + else 'incorrect' inputfield('string').fill(textvalue) elif problem_type == 'numerical': - textvalue = "pi + 1" if correctness == 'correct' else str(random.randint(-2, 2)) + textvalue = "pi + 1" if correctness == 'correct' \ + else str(random.randint(-2, 2)) inputfield('numerical').fill(textvalue) elif problem_type == 'formula': @@ -202,6 +213,7 @@ def answer_problem(step, problem_type, correctness): # Submit the problem check_problem(step) + @step(u'The "([^"]*)" problem displays a "([^"]*)" answer') def assert_problem_has_answer(step, problem_type, answer_class): ''' @@ -242,7 +254,8 @@ def assert_problem_has_answer(step, problem_type, answer_class): if answer_class == 'blank': expected = '' else: - expected = 'correct string' if answer_class == 'correct' else 'incorrect' + expected = 'correct string' if answer_class == 'correct' \ + else 'incorrect' assert_textfield('string', expected) @@ -286,7 +299,7 @@ CORRECTNESS_SELECTORS = { 'string': ['div.correct'], 'numerical': ['div.correct'], 'formula': ['div.correct'], - 'script': ['div.correct'], + 'script': ['div.correct'], 'code': ['span.correct']}, 'incorrect': {'drop down': ['span.incorrect'], @@ -306,12 +319,14 @@ CORRECTNESS_SELECTORS = { 'numerical': ['div.unanswered'], 'formula': ['div.unanswered'], 'script': ['div.unanswered'], - 'code': ['span.unanswered'] }} + 'code': ['span.unanswered']}} @step(u'My "([^"]*)" answer is marked "([^"]*)"') def assert_answer_mark(step, problem_type, correctness): - """ Assert that the expected answer mark is visible for a given problem type. + """ + Assert that the expected answer mark is visible + for a given problem type. *problem_type* is a string identifying the type of problem (e.g. 'drop down') *correctness* is in ['correct', 'incorrect', 'unanswered'] @@ -349,13 +364,14 @@ def inputfield(problem_type, choice=None, input_num=1): base = "_choice_" if problem_type == "multiple choice" else "_" sel = sel + base + str(choice) - + # If the input element doesn't exist, fail immediately assert(world.browser.is_element_present_by_css(sel, wait_time=4)) # Retrieve the input element return world.browser.find_by_css(sel) + def assert_checked(problem_type, choices): ''' Assert that choice names given in *choices* are the only @@ -365,14 +381,15 @@ def assert_checked(problem_type, choices): ''' all_choices = ['choice_0', 'choice_1', 'choice_2', 'choice_3'] - for ch in all_choices: - el = inputfield(problem_type, choice=ch) + for this_choice in all_choices: + element = inputfield(problem_type, choice=this_choice) - if ch in choices: - assert el.checked + if this_choice in choices: + assert element.checked else: - assert not el.checked + assert not element.checked + def assert_textfield(problem_type, expected_text, input_num=1): - el = inputfield(problem_type, input_num=input_num) - assert el.value == expected_text + element = inputfield(problem_type, input_num=input_num) + assert element.value == expected_text From d8f1c2b41a7b1f0af023f8dd75048f31d3df8569 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 22:49:39 -0400 Subject: [PATCH 6/7] add unit test for proper depth build out --- .../contentstore/tests/test_contentstore.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 615ffb6ed0..5b080c7a83 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -205,7 +205,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): new_loc = descriptor.location._replace(org='MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) - self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.status_code, 200) def test_delete_course(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) @@ -307,6 +307,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # note, we know the link it should be because that's what in the 'full' course in the test data self.assertContains(resp, '/c4x/edX/full/asset/handouts_schematic_tutorial.pdf') + def test_prefetch_children(self): + import_from_xml(modulestore(), 'common/test/data/', ['full']) + module_store = modulestore('direct') + location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + + course = module_store.get_item(location, depth=2) + + # make sure we pre-fetched a known sequential which should be at depth=2 + self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', + 'Administrivia_and_Circuit_Elements', None]) in course.system.module_data) + + # make sure we don't have a specific vertical which should be at depth=3 + self.assertFalse(Location(['i4x', 'edX', 'full', 'vertical', 'vertical_58', + None]) in course.system.module_data) + def test_export_course_with_unknown_metadata(self): module_store = modulestore('direct') content_store = contentstore() From 269152c4f2072decad55615c3fc7f612cce28075 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 23:15:35 -0400 Subject: [PATCH 7/7] add a test scenario to count RT to database when prefetching children. This uses a shim function on pymongo's collection.find to do the counting --- .../contentstore/tests/test_contentstore.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 5b080c7a83..edb20561bc 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -37,6 +37,14 @@ TEST_DATA_MODULESTORE = copy.deepcopy(settings.MODULESTORE) TEST_DATA_MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') TEST_DATA_MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') +class MongoCollectionFindWrapper(object): + def __init__(self, original): + self.original = original + self.counter = 0 + + def find(self, query, *args, **kwargs): + self.counter = self.counter+1 + return self.original(query, *args, **kwargs) @override_settings(MODULESTORE=TEST_DATA_MODULESTORE) class ContentStoreToyCourseTest(ModuleStoreTestCase): @@ -145,8 +153,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent no longer points to the child object which was deleted self.assertFalse(sequential.location.url() in chapter.children) - - def test_about_overrides(self): ''' This test case verifies that a course can use specialized override for about data, e.g. /about/Fall_2012/effort.html @@ -312,8 +318,15 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): module_store = modulestore('direct') location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + wrapper = MongoCollectionFindWrapper(module_store.collection.find) + module_store.collection.find = wrapper.find course = module_store.get_item(location, depth=2) + # make sure we haven't done too many round trips to DB + # note we say 4 round trips here for 1) the course, 2 & 3) for the chapters and sequentials, and + # 4) because of the RT due to calculating the inherited metadata + self.assertEqual(wrapper.counter, 4) + # make sure we pre-fetched a known sequential which should be at depth=2 self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]) in course.system.module_data)