Test PR | Enable Extracted Word Cloud #35983
Conversation
farhan
force-pushed
the
farhan/test-extracted-word-cloud
branch
from
8cf696a to
988cb0c
Compare
farhan
force-pushed
the
farhan/test-extracted-word-cloud
branch
4 times, most recently
from
ad5a8b7 to
02848a2
Compare
There was a problem hiding this comment.
I took a pass but since I'm not as familiar with the Xblock framework, forgive me if some of the questions have obvious answers. I'm in particular confused about why the tests need to change and if that indicates any sort of breakage that we need to handle elsewhere?
| 'element_id': self.block.location.html_id(), | ||
| '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: | ||
| expected_context['range_num_inputs'] = range(5) |
There was a problem hiding this comment.
Why not check the block type in this case?
There was a problem hiding this comment.
Fixed, checking for both (Builtin & Extracted) XBlocks now.
xmodule/tests/test_word_cloud.py
Outdated
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| 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) |
There was a problem hiding this comment.
Why do keys need to be provided for one instantiation but not the other?
There was a problem hiding this comment.
Update:
I have added keys for both builtin and extracted implementation to avoid If condition.
Why It's not needed in case of builtin xblock implementation?
In BuiltIn xblock implementation WordCloud block has XmlMixin and in its parse_xml method implementation, there is a check at start that if keys are None then it creates the keys.
I have copied the same lines of code and added them in test case.
In Extracted XBlock implementation parse_xml method of xblocks/core.py calls which doesn't have this check and expecting keys values in params.
There was a problem hiding this comment.
Further discussion:
#35983 (comment)
| # This will export the olx to a separate file. | ||
| block.add_xml_to_node(node) | ||
| if settings.USE_EXTRACTED_WORD_CLOUD_BLOCK: | ||
| filepath = 'word_cloud/block_id.xml' |
There was a problem hiding this comment.
Why not use the same method as below to export the file?
There was a problem hiding this comment.
Because add_xml_to_node is defined in XmlMixin class.
BuiltIn XBlocks are using this mixin
In Extracted XBlocks we are preferring to use Xblock.core functionalities and using export_to_xml method which doesn't have export_to_file funcationlity like add_xml_to_node method of XmlMixin has
@feanil Thanks for the pass. Let's keep this PR in draft until I make this PR and Extracted Word Cloud PR openedx/xblocks-contrib#4 fully ready. |
farhan
force-pushed
the
farhan/test-extracted-word-cloud
branch
from
3f0447a to
add935e
Compare
|
|
||
| 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) |
There was a problem hiding this comment.
As per discussion in pair programming session (@kdmccormick , @ttqureshi ) we are creating scope ids here which shouldn't happen and is a potential bug
farhan
force-pushed
the
farhan/test-extracted-word-cloud
branch
from
5768e3b to
d23a0a6
Compare
|
@feanil You can consider this PR to be ready for next pass |
kdmccormick
added
the
create-sandbox
|
Sandbox deployment failed 💥 |
It's the test PR for this xblocks-contrib repo PR
Steps done in this PR:
lms/envs/common.pysettingsfail-fast: falsein.github/workflows/unit-tests.ymlxblocks-contribwith following git branched installation.Fix all the failed test case ✅
PR sandbox
Username: openedx
PW: openedx
Settings