"masking temporarily disabled" >8 years ago: should we unskip or delete these tests?

[nedbat]

https://github.com/openedx/edx-platform/blob/71e2aa7018b3c9e461080c9501394ded447c768f/xmodule/tests/test_capa_block.py#L2042-L2082

These skips were added in commit 3ef1e0f.

[raju249]

I can work on this issue. Please assign this to me @nedbat

[feanil]

@raju249 FYI, if you leave a comment that says "assign me" that will also auto assign the issue to you.

[raju249]

I ran the masked tests and they failed with type errors as follows:

====================================================== FAILURES ======================================================
________________________________________ ProblemBlockTest.test_rescore_unmask ________________________________________

self = <xmodule.tests.test_capa_block.ProblemBlockTest testMethod=test_rescore_unmask>

    def test_rescore_unmask(self):
        """On problem rescore, unmasked names should appear on publish."""
        block = CapaFactory.create(xml=self.common_shuffle_xml)
        get_request_dict = {CapaFactory.input_key(): 'mask_0'}
        block.submit_problem(get_request_dict)
        # On rescore, state/student_answers should use unmasked names
        with patch.object(block.runtime, 'publish') as mock_publish:
>           block.rescore_problem(only_if_higher=False)  # lint-amnesty, pylint: disable=no-member
E           AttributeError: 'ProblemBlock' object has no attribute 'rescore_problem'

xmodule/tests/test_capa_block.py:2074: AttributeError
------------------------------------------------- Captured log call --------------------------------------------------
ERROR    edx.courseware:capa_block.py:1933 Unable to gather submission metadata, it will not be included in the event.
Traceback (most recent call last):
  File "/edx/app/edxapp/edx-platform/xmodule/capa_block.py", line 1928, in get_submission_metadata_safe
    return self.get_submission_metadata(answers, correct_map)
  File "/edx/app/edxapp/edx-platform/xmodule/capa_block.py", line 1978, in get_submission_metadata
    user_visible_answer = answer_input.get_user_visible_answer(internal_answer)
  File "/edx/app/edxapp/edx-platform/xmodule/capa/inputtypes.py", line 569, in get_user_visible_answer
    return self._choices_map[internal_answer]
KeyError: 'mask_0'
_________________________________________ ProblemBlockTest.test_save_unmask __________________________________________

self = <xmodule.tests.test_capa_block.ProblemBlockTest testMethod=test_save_unmask>

    def test_save_unmask(self):
        """On problem save, unmasked data should appear on publish."""
        block = CapaFactory.create(xml=self.common_shuffle_xml)
        with patch.object(block.runtime, 'publish') as mock_publish:
            get_request_dict = {CapaFactory.input_key(): 'mask_0'}
            block.save_problem(get_request_dict)
            mock_call = mock_publish.mock_calls[0]
            event_info = mock_call[1][1]
>           assert event_info['answers'][CapaFactory.answer_key()] == 'choice_2'
E           TypeError: string indices must be integers

xmodule/tests/test_capa_block.py:2050: TypeError
_________________________________________ ProblemBlockTest.test_reset_unmask _________________________________________

self = <xmodule.tests.test_capa_block.ProblemBlockTest testMethod=test_reset_unmask>

    def test_reset_unmask(self):
        """On problem reset, unmask names should appear publish."""
        block = CapaFactory.create(xml=self.common_shuffle_xml)
        get_request_dict = {CapaFactory.input_key(): 'mask_0'}
        block.submit_problem(get_request_dict)
        # On reset, 'old_state' should use unmasked names
        with patch.object(block.runtime, 'publish') as mock_publish:
            block.reset_problem(None)
            mock_call = mock_publish.mock_calls[0]
            event_info = mock_call[1][1]
>           assert mock_call[1][0] == 'reset_problem'
E           AssertionError: assert <ProblemBlock @789D parent=None, name=None, tags=[], display_name='Blank Advanced Problem', attempts=1, attempts_befor...source_code=None, student_answers={}, submission_wait_seconds=0, use_latex_compiler=False, weight=1, xml_attributes={}> == 'reset_problem'

xmodule/tests/test_capa_block.py:2063: AssertionError

I think we should probably remove them as I assume the implementation might have changed for the logic that these tests are running for.

[raju249]

I found that in #3872 we disabled masking, so until we enable it back, we can keep these tests but change them a bit to check for actual answers rather than masked ones getting unmasked.

This seems to be better option that deleting these tests. So that when we enable masking, we can change these tests again.

[feanil]

@nedbat I'd love your thoughts on this but given that they're failing and testing a feature that doesn't exist, I think it makes more sense to just delete the tests? It looked like the non-masked tests exist so having these run doesn't add extra value as far as I can tell?

[feanil]

@robrap , Ned suggested that you might be the right person to ask about this, do you have any thoughts?

[robrap]

My opinion:

1. I'd delete unused tests.
2. I'd update this ticket so TNL can decide to either: a) fix and restore capa name masking (whatever that is), or b) more permanently remove the feature and drop the "temporary" comments.

Given how long this has been gone, probably option (b)?

[feanil]

@raju249 can you update your PR to delete the unused tests and remove the comments that suggest we'll be putting masking back?