-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Allow for Extracted Word Cloud Block | Fix Test Cases #35983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
f89371b
8d622a4
a2d19bd
ba3df94
bfea346
c2c9b9e
af88397
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,17 @@ | ||
| """Word cloud integration tests using mongo modulestore.""" | ||
|
|
||
|
|
||
| import pytest | ||
|
|
||
| import json | ||
| import re | ||
| from operator import itemgetter | ||
| from unittest.mock import patch | ||
| from uuid import UUID | ||
|
|
||
| import pytest | ||
| from django.conf import settings | ||
|
|
||
| from common.djangoapps.student.tests.factories import RequestFactoryNoCsrf | ||
| # noinspection PyUnresolvedReferences | ||
| from xmodule.tests.helpers import override_descriptor_system # pylint: disable=unused-import | ||
| from xmodule.tests.helpers import override_descriptor_system, mock_render_template # pylint: disable=unused-import | ||
| from xmodule.x_module import STUDENT_VIEW | ||
|
|
||
| from .helpers import BaseTestXmodule | ||
|
|
||
|
|
||
|
|
@@ -18,6 +20,10 @@ class TestWordCloud(BaseTestXmodule): | |
| """Integration test for Word Cloud Block.""" | ||
| CATEGORY = "word_cloud" | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.request_factory = RequestFactoryNoCsrf() | ||
|
|
||
| def _get_users_state(self): | ||
| """Return current state for each user: | ||
|
|
||
|
|
@@ -27,7 +33,18 @@ def _get_users_state(self): | |
| users_state = {} | ||
|
|
||
| for user in self.users: | ||
| response = self.clients[user.username].post(self.get_url('get_state')) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # The extracted Word Cloud XBlock uses @XBlock.json_handler, which expects a different | ||
| # request format and url pattern | ||
| handler_url = self.get_url('', handler_name='handle_get_state') | ||
| response = self.clients[user.username].post( | ||
| handler_url, | ||
| data=json.dumps({}), | ||
| content_type='application/json', | ||
| HTTP_X_REQUESTED_WITH='XMLHttpRequest', | ||
| ) | ||
| else: | ||
| response = self.clients[user.username].post(self.get_url('get_state')) | ||
| users_state[user.username] = json.loads(response.content.decode('utf-8')) | ||
|
|
||
| return users_state | ||
|
|
@@ -40,19 +57,29 @@ def _post_words(self, words): | |
| users_state = {} | ||
|
|
||
| for user in self.users: | ||
| response = self.clients[user.username].post( | ||
| self.get_url('submit'), | ||
| {'student_words[]': words}, | ||
| HTTP_X_REQUESTED_WITH='XMLHttpRequest' | ||
| ) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # The extracted Word Cloud XBlock uses @XBlock.json_handler, which expects a different | ||
| # request format and url pattern | ||
| handler_url = self.get_url('', handler_name='handle_submit_state') | ||
| response = self.clients[user.username].post( | ||
| handler_url, | ||
| data=json.dumps({'student_words': words}), | ||
| content_type='application/json', | ||
| HTTP_X_REQUESTED_WITH='XMLHttpRequest', | ||
| ) | ||
| else: | ||
| response = self.clients[user.username].post( | ||
| self.get_url('submit'), | ||
| {'student_words[]': words}, | ||
| HTTP_X_REQUESTED_WITH='XMLHttpRequest' | ||
| ) | ||
| users_state[user.username] = json.loads(response.content.decode('utf-8')) | ||
|
|
||
| return users_state | ||
|
|
||
| def _check_response(self, response_contents, correct_jsons): | ||
| """Utility function that compares correct and real responses.""" | ||
| for username, content in response_contents.items(): | ||
|
|
||
| # Used in debugger for comparing objects. | ||
| # self.maxDiff = None | ||
|
|
||
|
|
@@ -120,7 +147,6 @@ def test_post_words(self): | |
|
|
||
| correct_state = {} | ||
| for index, user in enumerate(self.users): | ||
|
|
||
| correct_state[user.username] = { | ||
| 'status': 'success', | ||
| 'submitted': True, | ||
|
|
@@ -202,6 +228,14 @@ def test_handle_ajax_incorrect_dispatch(self): | |
| for user in self.users | ||
| } | ||
|
|
||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # The extracted Word Cloud XBlock uses @XBlock.json_handler to handle AJAX requests, | ||
| # which automatically returns a 404 for unknown requests, so there's no need to test | ||
| # the incorrect dispatch case in this scenario. | ||
| for username, response in responses.items(): | ||
| self.assertEqual(response.status_code, 404) | ||
| return | ||
|
|
||
| status_codes = {response.status_code for response in responses.values()} | ||
| assert status_codes.pop() == 200 | ||
|
|
||
|
|
@@ -214,19 +248,34 @@ def test_handle_ajax_incorrect_dispatch(self): | |
| } | ||
| ) | ||
|
|
||
| def test_word_cloud_constructor(self): | ||
| @patch('xblock.utils.resources.ResourceLoader.render_django_template', side_effect=mock_render_template) | ||
| def test_word_cloud_constructor(self, mock_render_django_template): | ||
| """ | ||
| Make sure that all parameters extracted correctly from xml. | ||
| """ | ||
| fragment = self.runtime.render(self.block, STUDENT_VIEW) | ||
| expected_context = { | ||
| 'ajax_url': self.block.ajax_url, | ||
| 'display_name': self.block.display_name, | ||
| 'instructions': self.block.instructions, | ||
| 'element_class': self.block.location.block_type, | ||
| 'element_id': self.block.location.html_id(), | ||
| 'element_class': self.block.scope_ids.block_type, | ||
| 'num_inputs': 5, # default value | ||
| 'submitted': False, # default value, | ||
| } | ||
|
|
||
| assert fragment.content == self.runtime.render_template('word_cloud.html', expected_context) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # If `USE_EXTRACTED_WORD_CLOUD_BLOCK` is enabled, the `expected_context` will be different | ||
| # because in the extracted Word Cloud XBlock, the expected context: | ||
| # - contains `range_num_inputs` | ||
| # - uses `UUID` for `element_id` instead of `html_id()` | ||
| # - does not include `ajax_url` since it uses the `@XBlock.json_handler` decorator for AJAX requests | ||
| expected_context['range_num_inputs'] = range(5) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not check the block type in this case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, checking for both (Builtin & Extracted) XBlocks now. |
||
| uuid_str = re.search(r"UUID\('([a-f0-9\-]+)'\)", fragment.content).group(1) | ||
| expected_context['element_id'] = UUID(uuid_str) | ||
| mock_render_django_template.assert_called_once() | ||
| # Remove i18n service | ||
| fragment_content_clean = re.sub(r"\{.*?}", "{}", fragment.content) | ||
| assert fragment_content_clean == self.runtime.render_template('templates/word_cloud.html', expected_context) | ||
| else: | ||
| expected_context['ajax_url'] = self.block.ajax_url | ||
| expected_context['element_id'] = self.block.location.html_id() | ||
| assert fragment.content == self.runtime.render_template('word_cloud.html', expected_context) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,18 @@ | ||
| """Test for Word Cloud Block functional logic.""" | ||
|
|
||
| import json | ||
| import os | ||
| from unittest.mock import Mock | ||
|
|
||
| from django.conf import settings | ||
| from django.test import TestCase | ||
| from fs.memoryfs import MemoryFS | ||
| from lxml import etree | ||
| from webob import Request | ||
| from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator | ||
| from webob.multidict import MultiDict | ||
| from xblock.field_data import DictFieldData | ||
| from xblock.fields import ScopeIds | ||
|
|
||
| from xmodule.word_cloud_block import WordCloudBlock | ||
| from . import get_test_descriptor_system, get_test_system | ||
|
|
@@ -43,7 +46,11 @@ def test_xml_import_export_cycle(self): | |
|
|
||
| olx_element = etree.fromstring(original_xml) | ||
| runtime.id_generator = Mock() | ||
| block = WordCloudBlock.parse_xml(olx_element, runtime, None) | ||
|
|
||
| def_id = runtime.id_generator.create_definition(olx_element.tag, olx_element.get('url_name')) | ||
| keys = ScopeIds(None, olx_element.tag, def_id, runtime.id_generator.create_usage(def_id)) | ||
| block = WordCloudBlock.parse_xml(olx_element, runtime, keys) | ||
|
farhan marked this conversation as resolved.
Outdated
|
||
|
|
||
| block.location = BlockUsageLocator( | ||
| CourseLocator('org', 'course', 'run', branch='revision'), 'word_cloud', 'block_id' | ||
| ) | ||
|
|
@@ -54,18 +61,41 @@ def test_xml_import_export_cycle(self): | |
| assert block.num_inputs == 3 | ||
| assert block.num_top_words == 100 | ||
|
|
||
| node = etree.Element("unknown_root") | ||
| # This will export the olx to a separate file. | ||
| block.add_xml_to_node(node) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # For extracted XBlocks, we need to manually export the XML definition to a file to properly test the | ||
| # import/export cycle. This is because extracted XBlocks use XBlock core's `add_xml_to_node` method, | ||
| # which does not export the XML to a file like `XmlMixin.add_xml_to_node` does. | ||
| filepath = 'word_cloud/block_id.xml' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the same method as below to export the file?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because add_xml_to_node is defined in XmlMixin class. In Extracted XBlocks we are preferring to use Xblock.core functionalities and using export_to_xml method which doesn't have |
||
| runtime.export_fs.makedirs(os.path.dirname(filepath), recreate=True) | ||
| with runtime.export_fs.open(filepath, 'wb') as fileObj: | ||
| runtime.export_to_xml(block, fileObj) | ||
| else: | ||
| node = etree.Element("unknown_root") | ||
| # This will export the olx to a separate file. | ||
| block.add_xml_to_node(node) | ||
|
|
||
| with runtime.export_fs.open('word_cloud/block_id.xml') as f: | ||
| exported_xml = f.read() | ||
|
|
||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # For extracted XBlocks, we need to remove the `xblock-family` attribute from the exported XML to ensure | ||
| # consistency with the original XML. | ||
| # This is because extracted XBlocks use the core XBlock's `add_xml_to_node` method, which includes this | ||
| # attribute, whereas `XmlMixin.add_xml_to_node` does not. | ||
| exported_xml_tree = etree.fromstring(exported_xml.encode('utf-8')) | ||
| etree.cleanup_namespaces(exported_xml_tree) | ||
| if 'xblock-family' in exported_xml_tree.attrib: | ||
| del exported_xml_tree.attrib['xblock-family'] | ||
| exported_xml = etree.tostring(exported_xml_tree, encoding='unicode', pretty_print=True) | ||
|
|
||
| assert exported_xml == original_xml | ||
|
|
||
| def test_bad_ajax_request(self): | ||
| """ | ||
| Make sure that answer for incorrect request is error json. | ||
| """ | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| return | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is essentially just skipping the test! Could you run as much of the test as possible without the if, and then just use the if statement at the end for the final assertion? |
||
|
|
||
| module_system = get_test_system() | ||
| block = WordCloudBlock(module_system, DictFieldData(self.raw_field_data), Mock()) | ||
|
|
@@ -84,8 +114,14 @@ def test_good_ajax_request(self): | |
| module_system = get_test_system() | ||
| block = WordCloudBlock(module_system, DictFieldData(self.raw_field_data), Mock()) | ||
|
|
||
| post_data = MultiDict(('student_words[]', word) for word in ['cat', 'cat', 'dog', 'sun']) | ||
| response = json.loads(block.handle_ajax('submit', post_data)) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # The extracted Word Cloud XBlock uses @XBlock.json_handler for handling AJAX requests. | ||
| # It expects a standard Python dictionary as POST data and returns a JSON object in response. | ||
| post_data = {'student_words': ['cat', 'cat', 'dog', 'sun']} | ||
| response = block.submit_state(post_data) | ||
| else: | ||
| post_data = MultiDict(('student_words[]', word) for word in ['cat', 'cat', 'dog', 'sun']) | ||
| response = json.loads(block.handle_ajax('submit', post_data)) | ||
| assert response['status'] == 'success' | ||
| assert response['submitted'] is True | ||
| assert response['total_count'] == 22 | ||
|
|
@@ -128,13 +164,23 @@ def test_studio_submit_handler(self): | |
| 'num_top_words': 10, | ||
| 'display_student_percents': 'False', | ||
| } | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| # In the extracted Word Cloud XBlock, we use StudioEditableXBlockMixin.submit_studio_edits, | ||
| # which expects a different handler name and request JSON format. | ||
| handler_name = 'submit_studio_edits' | ||
| TEST_REQUEST_JSON = { | ||
| 'values': TEST_SUBMIT_DATA, | ||
| } | ||
| else: | ||
| handler_name = 'studio_submit' | ||
| TEST_REQUEST_JSON = TEST_SUBMIT_DATA | ||
| module_system = get_test_system() | ||
| block = WordCloudBlock(module_system, DictFieldData(self.raw_field_data), Mock()) | ||
| body = json.dumps(TEST_SUBMIT_DATA) | ||
| body = json.dumps(TEST_REQUEST_JSON) | ||
| request = Request.blank('/') | ||
| request.method = 'POST' | ||
| request.body = body.encode('utf-8') | ||
| res = block.handle('studio_submit', request) | ||
| res = block.handle(handler_name, request) | ||
| assert json.loads(res.body.decode('utf8')) == {'result': 'success'} | ||
|
|
||
| assert block.display_name == TEST_SUBMIT_DATA['display_name'] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ensure that these tests all run on CI for both the built-in block and the extracted block, regardless of the default setting value.
Here's a way to achieve this without having to change many lines:
The current test class is renamed to be just a base mixin (the "Mixin" suffix tells pytest not to execute the tests cases on this class):
and then you have one concrete child class for the built-in block and one for the extracted block, both of which are empty (they'll inherit all their test cases from _TestWordCloudMixin):
How does that look? I think this should work both for this tests class and for the other WordCloud test class below, and I believe it will also work for other blocks like Poll once they're ready too.