-
Notifications
You must be signed in to change notification settings - Fork 14
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
test: [AXM-636] Cover Offline Mode API with unit tests #2577
test: [AXM-636] Cover Offline Mode API with unit tests #2577
Conversation
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.
Good work, but can you please tell me if it is possible to add some more functional tests to OfflineModeUtilsTestCase
and AssetsManagementTestCase
to be more confident that the new functionality works as expected?
@patch('lms.djangoapps.mobile_api.course_info.views.default_storage') | ||
@patch('lms.djangoapps.mobile_api.course_info.views.get_offline_block_content_path') | ||
@patch('lms.djangoapps.mobile_api.course_info.views.is_offline_mode_enabled') | ||
def test_extend_block_info_with_offline_data( |
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.
I think it's worth adding a test to check for the absence of a new field in version 3 of this API
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.
Added tests for versions less then v4
|
||
@patch('openedx.features.offline_mode.html_manipulator.save_asset_file') | ||
def test_replace_asset_links(self, save_asset_file_mock: MagicMock) -> None: | ||
html_data_mock = '<div class="teacher-image"><img src="/assets/images/professor-sandel.jpg"/></div>' |
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.
Please change this path to a more plausible one that contains asset_key
in its path.
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.
fixed
result = xblock_renderer.render_xblock_from_lms() | ||
|
||
self.assertIsNotNone(result) | ||
self.assertEqual(type(result), str) |
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.
Should we add something like assert "<p>Test HTML Content<p>" in result
?
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.
Added additional checks here
result = xblock_renderer.render_xblock_from_lms() | ||
|
||
self.assertIsNotNone(result) | ||
self.assertEqual(type(result), str) |
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.
Here the same.
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.
And here
) | ||
|
||
|
||
class GenerateOfflineContentTasksTestCase(TestCase): |
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.
Can we test the mechanism of retrying the task in case of errors?
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.
Added tests for the testing retrying mechanism
from rest_framework import status | ||
|
||
|
||
class SudioCoursePublishedEventHandlerTestCase(TestCase): |
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.
I added functional tests to this view in my PR, so we can actually remove them here.
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.
removed
071f756
to
f0e5fee
Compare
It is possible. But there are some problems with test storage. Need more time for investigation |
f0e5fee
to
5379d59
Compare
Co-authored-by: Ivan Niedielnitsev <[email protected]>
d0da62d
to
fc5be32
Compare
Description
Cover Offline Mode API with unit tests, functional tests and integration tests.
To run all tests type next command in LMS container:
pytest openedx/features/offline_mode
YouTrack
https://youtrack.raccoongang.com/issue/AXM-636/Unit-tests-Offline-mode
https://youtrack.raccoongang.com/issue/AXM-653