From 4f9d18df8ccd00135298a6d728d4f2ade3725424 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 29 Mar 2013 11:59:08 -0400 Subject: [PATCH 1/6] Wrote unit tests to verify bug fix in https://github.com/MITx/mitx/pull/1764 --- .../xmodule/xmodule/tests/test_capa_module.py | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index d2458cb3d0..1b923c13f8 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -859,3 +859,62 @@ class CapaModuleTest(unittest.TestCase): # Expect that the module has created a new dummy problem with the error self.assertNotEqual(original_problem, module.lcp) + + + def test_random_seed_no_change(self): + rerandomize_options = ['never', 'per_student', 'always', 'onreset'] + + for rerandomize in rerandomize_options: + module = CapaFactory.create(rerandomize=rerandomize) + + # Get the seed + # module.seed isn't set until the problem is checked/saved, + # so we access the capa problem seed directly + seed = module.lcp.seed + self.assertTrue(seed is not None) + + if rerandomize == 'never': + self.assertEqual(seed, 1) + + # Check the problem + get_request_dict = { CapaFactory.input_key(): '3.14'} + module.check_problem(get_request_dict) + + # Expect that the seed is the same + self.assertEqual(seed, module.seed) + + # Save the problem + module.save_problem(get_request_dict) + + # Expect that the seed is the same + self.assertEqual(seed, module.seed) + + def test_random_seed_with_reset(self): + rerandomize_options = ['never', 'per_student', 'always', 'onreset'] + + for rerandomize in rerandomize_options: + module = CapaFactory.create(rerandomize=rerandomize) + + # Get the seed + seed = module.lcp.seed + + # Reset the problem + module.reset_problem({}) + + # We do NOT want the seed to reset if rerandomize + # is set to 'never' -- it should still be 1 + # The seed also stays the same if we're randomizing + # 'per_student': the same student should see the same problem + if rerandomize in ['never', 'per_student']: + self.assertEqual(seed, module.seed) + + # Otherwise, we expect the seed to change + # to another valid seed + else: + + # After we save, the seed is stored in the module + get_request_dict = { CapaFactory.input_key(): '3.14'} + module.save_problem(get_request_dict) + + self.assertEqual(seed, module.seed) + self.assertTrue(module.seed is not None) From f90e29ce966d7524a9df0409729fb7be743eb933 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 1 Apr 2013 08:55:06 -0400 Subject: [PATCH 2/6] Fixed a test error that occurred because the problem was not set to done before resetting it. To avoid to small chance that we'll get the same seed when we reset, try to get a new seed multiple times. --- .../xmodule/xmodule/tests/test_capa_module.py | 53 ++++++++++++++----- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 1b923c13f8..1ebb83d2e4 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -890,31 +890,60 @@ class CapaModuleTest(unittest.TestCase): self.assertEqual(seed, module.seed) def test_random_seed_with_reset(self): - rerandomize_options = ['never', 'per_student', 'always', 'onreset'] + + def _reset_and_get_seed(module): + ''' + Reset the XModule and return the module's seed + ''' - for rerandomize in rerandomize_options: + # Simulate submitting an attempt + module.done = True + + # Reset the problem + module.reset_problem({}) + + # Get return the seed + return module.seed + + def _retry_and_check(num_tries, test_func): + ''' + Returns True if *test_func* was successful + (returned True) within *num_tries* attempts + + *test_func* must be a function + of the form test_func() -> bool + ''' + success = False + for i in range(num_tries): + if test_func() is True: + success = True + break + return success + + # Run the test for each possible rerandomize value + for rerandomize in ['never', 'per_student', 'always', 'onreset']: module = CapaFactory.create(rerandomize=rerandomize) # Get the seed seed = module.lcp.seed - # Reset the problem - module.reset_problem({}) - # We do NOT want the seed to reset if rerandomize # is set to 'never' -- it should still be 1 # The seed also stays the same if we're randomizing # 'per_student': the same student should see the same problem if rerandomize in ['never', 'per_student']: - self.assertEqual(seed, module.seed) + self.assertEqual(seed, _reset_and_get_seed(module)) # Otherwise, we expect the seed to change # to another valid seed else: - - # After we save, the seed is stored in the module - get_request_dict = { CapaFactory.input_key(): '3.14'} - module.save_problem(get_request_dict) - self.assertEqual(seed, module.seed) - self.assertTrue(module.seed is not None) + # Since there's a small chance we might get the + # same seed again, give it 5 chances + # to generate a different seed + success = _retry_and_check(5, + lambda: _reset_and_get_seed(module) != seed) + + self.assertTrue(module.seed != None) + msg = 'Could not get a new seed from reset after 5 tries' + self.assertTrue(success, msg) From 4322b83894c0d729c0a857fcc745b4df45bb0121 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 1 Apr 2013 09:02:04 -0400 Subject: [PATCH 3/6] Added additional comments --- .../lib/xmodule/xmodule/tests/test_capa_module.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 1ebb83d2e4..08565e97ad 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -862,9 +862,9 @@ class CapaModuleTest(unittest.TestCase): def test_random_seed_no_change(self): - rerandomize_options = ['never', 'per_student', 'always', 'onreset'] - for rerandomize in rerandomize_options: + # Run the test for each possible rerandomize value + for rerandomize in ['never', 'per_student', 'always', 'onreset']: module = CapaFactory.create(rerandomize=rerandomize) # Get the seed @@ -873,6 +873,8 @@ class CapaModuleTest(unittest.TestCase): seed = module.lcp.seed self.assertTrue(seed is not None) + # If we're not rerandomizing, the seed is always set + # to the same value (1) if rerandomize == 'never': self.assertEqual(seed, 1) @@ -897,12 +899,15 @@ class CapaModuleTest(unittest.TestCase): ''' # Simulate submitting an attempt + # We need to do this, or reset_problem() will + # fail with a complaint that we haven't submitted + # the problem yet. module.done = True # Reset the problem module.reset_problem({}) - # Get return the seed + # Return the seed return module.seed def _retry_and_check(num_tries, test_func): @@ -925,6 +930,8 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed + # module.seed isn't set until we check/save/reset the problem, + # so we access the capa problem seed directly seed = module.lcp.seed # We do NOT want the seed to reset if rerandomize From b76adb4b0681b1d1d71ca7bc76f812ca4c711ea4 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 1 Apr 2013 11:22:42 -0400 Subject: [PATCH 4/6] Updated tests to address on-going randomization bug. Capa problem state was not being copied to module state during module initialization. This meant that when check_problem was called the first time, the seed would be re-generated and saved. --- common/lib/xmodule/xmodule/capa_module.py | 4 +++- common/lib/xmodule/xmodule/tests/test_capa_module.py | 8 ++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index da8b5b4f96..0e6d81bdeb 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -171,7 +171,9 @@ class CapaModule(CapaFields, XModule): # add extra info and raise raise Exception(msg), None, sys.exc_info()[2] - self.set_state_from_lcp() + # Ensure that the module state matches the state of the + # capa problem + self.set_state_from_lcp() def new_lcp(self, state, text=None): if text is None: diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 08565e97ad..fa6d7ce724 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -868,9 +868,7 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - # module.seed isn't set until the problem is checked/saved, - # so we access the capa problem seed directly - seed = module.lcp.seed + seed = module.seed self.assertTrue(seed is not None) # If we're not rerandomizing, the seed is always set @@ -930,9 +928,7 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - # module.seed isn't set until we check/save/reset the problem, - # so we access the capa problem seed directly - seed = module.lcp.seed + seed = module.seed # We do NOT want the seed to reset if rerandomize # is set to 'never' -- it should still be 1 From 7f0048720377141b05ee1e90cb0070ca6632cd95 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 1 Apr 2013 11:48:10 -0400 Subject: [PATCH 5/6] Reverted last commit due to concerns about database writes. --- common/lib/xmodule/xmodule/capa_module.py | 4 +--- common/lib/xmodule/xmodule/tests/test_capa_module.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 0e6d81bdeb..da8b5b4f96 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -171,9 +171,7 @@ class CapaModule(CapaFields, XModule): # add extra info and raise raise Exception(msg), None, sys.exc_info()[2] - # Ensure that the module state matches the state of the - # capa problem - self.set_state_from_lcp() + self.set_state_from_lcp() def new_lcp(self, state, text=None): if text is None: diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index fa6d7ce724..60e51fd725 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -868,7 +868,10 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - seed = module.seed + # This isn't stored in the XModule until + # we check/save/reset the problem, so we + # access the lcp seed directly + seed = module.lcp.seed self.assertTrue(seed is not None) # If we're not rerandomizing, the seed is always set @@ -928,7 +931,10 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - seed = module.seed + # This isn't stored in the XModule until + # we check/save/reset the problem, so we + # access the lcp seed directly + seed = module.lcp.seed # We do NOT want the seed to reset if rerandomize # is set to 'never' -- it should still be 1 From fb9760244262073384c90ec0de29e737455c5ce8 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 1 Apr 2013 12:08:53 -0400 Subject: [PATCH 6/6] Save randomization seed if it is not already set, so that the same problem loads when the user checks/saves/resets --- common/lib/xmodule/xmodule/capa_module.py | 10 ++++++++++ .../lib/xmodule/xmodule/tests/test_capa_module.py | 13 +++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index da8b5b4f96..0869c3e484 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -150,6 +150,16 @@ class CapaModule(CapaFields, XModule): # TODO (vshnayder): move as much as possible of this work and error # checking to descriptor load time self.lcp = self.new_lcp(self.get_state_for_lcp()) + + # At this point, we need to persist the randomization seed + # so that when the problem is re-loaded (to check/view/save) + # it stays the same. + # However, we do not want to write to the database + # every time the module is loaded. + # So we set the seed ONLY when there is not one set already + if self.seed is None: + self.seed = self.lcp.seed + except Exception as err: msg = 'cannot create LoncapaProblem {loc}: {err}'.format( loc=self.location.url(), err=err) diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 60e51fd725..c9053be2ad 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -868,10 +868,8 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - # This isn't stored in the XModule until - # we check/save/reset the problem, so we - # access the lcp seed directly - seed = module.lcp.seed + # By this point, the module should have persisted the seed + seed = module.seed self.assertTrue(seed is not None) # If we're not rerandomizing, the seed is always set @@ -931,10 +929,9 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(rerandomize=rerandomize) # Get the seed - # This isn't stored in the XModule until - # we check/save/reset the problem, so we - # access the lcp seed directly - seed = module.lcp.seed + # By this point, the module should have persisted the seed + seed = module.seed + self.assertTrue(seed is not None) # We do NOT want the seed to reset if rerandomize # is set to 'never' -- it should still be 1