diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 45200b8607..1c0189d9aa 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -17,9 +17,8 @@ from datetime import datetime import logging import math import numpy -import os, os.path +import os.path import re -import struct import sys from lxml import etree @@ -73,7 +72,7 @@ class LoncapaProblem(object): - problem_text (string): xml defining the problem - id (string): identifier for this problem; often a filename (no spaces) - - seed (int): random number generator seed (int) + - seed (int): random number generator seed (int) - state (dict): containing the following keys: - 'seed' - (int) random number generator seed - 'student_answers' - (dict) maps input id to the stored answer for that input @@ -92,23 +91,20 @@ class LoncapaProblem(object): if self.system is None: raise Exception() - state = state if state else {} + state = state or {} # Set seed according to the following priority: # 1. Contained in problem's state # 2. Passed into capa_problem via constructor - # 3. Assign from the OS's random number generator self.seed = state.get('seed', seed) - if self.seed is None: - self.seed = struct.unpack('i', os.urandom(4))[0] + assert self.seed is not None, "Seed must be provided for LoncapaProblem." + self.student_answers = state.get('student_answers', {}) if 'correct_map' in state: self.correct_map.set_dict(state['correct_map']) self.done = state.get('done', False) self.input_state = state.get('input_state', {}) - - # Convert startouttext and endouttext to proper problem_text = re.sub("startouttext\s*/", "text", problem_text) problem_text = re.sub("endouttext\s*/", "/text", problem_text) diff --git a/common/lib/capa/capa/tests/__init__.py b/common/lib/capa/capa/tests/__init__.py index 59c87780b5..1997e4d055 100644 --- a/common/lib/capa/capa/tests/__init__.py +++ b/common/lib/capa/capa/tests/__init__.py @@ -1,7 +1,7 @@ -import fs import fs.osfs -import os +import os, os.path +from capa.capa_problem import LoncapaProblem from mock import Mock, MagicMock import xml.sax.saxutils as saxutils @@ -36,3 +36,7 @@ test_system = Mock( anonymous_student_id='student', cache=None, ) + +def new_loncapa_problem(xml): + """Construct a `LoncapaProblem` suitable for unit tests.""" + return LoncapaProblem(xml, id='1', seed=723, system=test_system) diff --git a/common/lib/capa/capa/tests/test_html_render.py b/common/lib/capa/capa/tests/test_html_render.py index 492fcb2743..e0edf344f2 100644 --- a/common/lib/capa/capa/tests/test_html_render.py +++ b/common/lib/capa/capa/tests/test_html_render.py @@ -6,9 +6,8 @@ import json import mock -from capa.capa_problem import LoncapaProblem from .response_xml_factory import StringResponseXMLFactory, CustomResponseXMLFactory -from . import test_system +from . import test_system, new_loncapa_problem class CapaHtmlRenderTest(unittest.TestCase): @@ -20,7 +19,7 @@ class CapaHtmlRenderTest(unittest.TestCase): xml_str = " " # Create the problem - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Render the HTML rendered_html = etree.XML(problem.get_html()) @@ -39,7 +38,7 @@ class CapaHtmlRenderTest(unittest.TestCase): """) # Create the problem - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Render the HTML rendered_html = etree.XML(problem.get_html()) @@ -61,7 +60,7 @@ class CapaHtmlRenderTest(unittest.TestCase): """) # Create the problem - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Render the HTML rendered_html = etree.XML(problem.get_html()) @@ -80,7 +79,7 @@ class CapaHtmlRenderTest(unittest.TestCase): """) # Create the problem - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Render the HTML rendered_html = etree.XML(problem.get_html()) @@ -98,7 +97,7 @@ class CapaHtmlRenderTest(unittest.TestCase): """) # Create the problem - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Render the HTML rendered_html = etree.XML(problem.get_html()) @@ -121,7 +120,7 @@ class CapaHtmlRenderTest(unittest.TestCase): test_system.render_template.return_value = "
Input Template Render
" # Create the problem and render the HTML - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) rendered_html = etree.XML(problem.get_html()) # Expect problem has been turned into a
@@ -184,7 +183,7 @@ class CapaHtmlRenderTest(unittest.TestCase): xml_str = CustomResponseXMLFactory().build_xml(**kwargs) # Create the problem and render the html - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) # Grade the problem correctmap = problem.grade_answers({'1_2_1': 'test'}) @@ -219,7 +218,7 @@ class CapaHtmlRenderTest(unittest.TestCase): """) # Create the problem and render the HTML - problem = LoncapaProblem(xml_str, '1', system=test_system) + problem = new_loncapa_problem(xml_str) rendered_html = etree.XML(problem.get_html()) # Expect that the variable $test has been replaced with its value diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 8e41ff39de..9cafef23d6 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -12,9 +12,8 @@ import textwrap import mock import textwrap -from . import test_system +from . import new_loncapa_problem -import capa.capa_problem as lcp from capa.responsetypes import LoncapaProblemError, \ StudentInputError, ResponseError from capa.correctmap import CorrectMap @@ -33,7 +32,7 @@ class ResponseTest(unittest.TestCase): def build_problem(self, **kwargs): xml = self.xml_factory.build_xml(**kwargs) - return lcp.LoncapaProblem(xml, '1', system=test_system) + return new_loncapa_problem(xml) def assert_grade(self, problem, submission, expected_correctness, msg=None): input_dict = {'1_2_1': submission} diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 4143345196..ec0c3c8619 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -3,7 +3,9 @@ import datetime import hashlib import json import logging +import os import traceback +import struct import sys from pkg_resources import resource_string @@ -23,8 +25,10 @@ from xmodule.util.date_utils import time_to_datetime log = logging.getLogger("mitx.courseware") -# Generated this many different variants of problems with rerandomize=per_student +# Generate this many different variants of problems with rerandomize=per_student NUM_RANDOMIZATION_BINS = 20 +# Never produce more than this many different seeds, no matter what. +MAX_RANDOMIZATION_BINS = 1000 def randomization_bin(seed, problem_id): @@ -108,11 +112,7 @@ class CapaModule(CapaFields, XModule): self.close_date = due_date if self.seed is None: - if self.rerandomize == 'never': - self.seed = 1 - elif self.rerandomize == "per_student" and hasattr(self.system, 'seed'): - # see comment on randomization_bin - self.seed = randomization_bin(system.seed, self.location.url) + self.choose_new_seed() # Need the problem location in openendedresponse to send out. Adding # it to the system here seems like the least clunky way to get it @@ -156,6 +156,22 @@ class CapaModule(CapaFields, XModule): self.set_state_from_lcp() + assert self.seed is not None + + def choose_new_seed(self): + """Choose a new seed.""" + if self.rerandomize == 'never': + self.seed = 1 + elif self.rerandomize == "per_student" and hasattr(self.system, 'seed'): + # see comment on randomization_bin + self.seed = randomization_bin(self.system.seed, self.location.url) + else: + self.seed = struct.unpack('i', os.urandom(4))[0] + + # So that sandboxed code execution can be cached, but still have an interesting + # number of possibilities, cap the number of different random seeds. + self.seed %= MAX_RANDOMIZATION_BINS + def new_lcp(self, state, text=None): if text is None: text = self.data @@ -164,6 +180,7 @@ class CapaModule(CapaFields, XModule): problem_text=text, id=self.location.html_id(), state=state, + seed=self.seed, system=self.system, ) @@ -831,14 +848,11 @@ class CapaModule(CapaFields, XModule): 'error': "Refresh the page and make an attempt before resetting."} if self.rerandomize in ["always", "onreset"]: - # reset random number generator seed (note the self.lcp.get_state() - # in next line) - seed = None - else: - seed = self.lcp.seed + # Reset random number generator seed. + self.choose_new_seed() # Generate a new problem with either the previous seed or a new seed - self.lcp = self.new_lcp({'seed': seed}) + self.lcp = self.new_lcp(None) # Pull in the new problem seed self.set_state_from_lcp() diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index f948f5bdfe..61de21b129 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -550,6 +550,7 @@ class CapaModuleTest(unittest.TestCase): def test_reset_problem(self): module = CapaFactory.create(done=True) module.new_lcp = Mock(wraps=module.new_lcp) + module.choose_new_seed = Mock(wraps=module.choose_new_seed) # Stub out HTML rendering with patch('xmodule.capa_module.CapaModule.get_problem_html') as mock_html: @@ -567,7 +568,8 @@ class CapaModuleTest(unittest.TestCase): self.assertEqual(result['html'], "
Test HTML
") # Expect that the problem was reset - module.new_lcp.assert_called_once_with({'seed': None}) + module.new_lcp.assert_called_once_with(None) + module.choose_new_seed.assert_called_once_with() def test_reset_problem_closed(self): module = CapaFactory.create() @@ -1033,3 +1035,13 @@ class CapaModuleTest(unittest.TestCase): self.assertTrue(module.seed is not None) msg = 'Could not get a new seed from reset after 5 tries' self.assertTrue(success, msg) + + def test_random_seed_bins(self): + # Assert that we are limiting the number of possible seeds. + + # Check the conditions that generate random seeds + for rerandomize in ['always', 'per_student', 'true', 'onreset']: + # Get a bunch of seeds, they should all be in 0-999. + for i in range(200): + module = CapaFactory.create(rerandomize=rerandomize) + assert 0 <= module.seed < 1000