Skip to content
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: run ./xmodule/ tests with CMS settings too #33534

Merged
merged 1 commit into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions .github/workflows/unit-test-shards.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"lms/tests.py"
]
},
"openedx-1": {
"openedx-1-with-lms": {
"settings": "lms.envs.test",
"paths": [
"openedx/core/djangoapps/ace_common/",
Expand Down Expand Up @@ -116,7 +116,7 @@
"openedx/core/djangoapps/external_user_ids/"
]
},
"openedx-2": {
"openedx-2-with-lms": {
"settings": "lms.envs.test",
"paths": [
"openedx/core/djangoapps/geoinfo/",
Expand Down Expand Up @@ -159,7 +159,7 @@
"openedx/tests/"
]
},
"openedx-3": {
"openedx-1-with-cms": {
"settings": "cms.envs.test",
"paths": [
"openedx/core/djangoapps/ace_common/",
Expand Down Expand Up @@ -197,7 +197,7 @@
"openedx/core/djangoapps/external_user_ids/"
]
},
"openedx-4": {
"openedx-2-with-cms": {
"settings": "cms.envs.test",
"paths": [
"openedx/core/djangoapps/content_tagging/",
Expand Down Expand Up @@ -258,22 +258,28 @@
"cms/djangoapps/contentstore/"
]
},
"common-1": {
"common-with-lms": {
"settings": "lms.envs.test",
"paths": [
"common/djangoapps/"
]
},
"common-2": {
"common-with-cms": {
"settings": "cms.envs.test",
"paths": [
"common/djangoapps/"
]
},
"xmodule-1": {
"xmodule-with-lms": {
"settings": "lms.envs.test",
"paths": [
"xmodule/"
]
},
"xmodule-with-cms": {
"settings": "cms.envs.test",
"paths": [
"xmodule/"
]
}
}
15 changes: 8 additions & 7 deletions .github/workflows/unit-tests-gh-hosted.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,16 @@ jobs:
- "lms-4"
- "lms-5"
- "lms-6"
- "openedx-1"
- "openedx-2"
- "openedx-3"
- "openedx-4"
- "openedx-1-with-lms"
- "openedx-2-with-lms"
- "openedx-1-with-cms"
- "openedx-2-with-cms"
- "cms-1"
- "cms-2"
- "common-1"
- "common-2"
- "xmodule-1"
- "common-with-lms"
- "common-with-cms"
- "xmodule-with-lms"
- "xmodule-with-cms"
name: gh-hosted-python-${{ matrix.python-version }},django-${{ matrix.django-version }},${{ matrix.shard_name }}
steps:
- uses: actions/checkout@v2
Expand Down
15 changes: 8 additions & 7 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,16 @@ jobs:
- "lms-4"
- "lms-5"
- "lms-6"
- "openedx-1"
- "openedx-2"
- "openedx-3"
- "openedx-4"
- "openedx-1-with-lms"
- "openedx-2-with-lms"
- "openedx-1-with-cms"
- "openedx-2-with-cms"
- "cms-1"
- "cms-2"
- "common-1"
- "common-2"
- "xmodule-1"
- "common-with-lms"
- "common-with-cms"
- "xmodule-with-lms"
- "xmodule-with-cms"
# We expect Django 4.0 to fail, so don't stop when it fails.
continue-on-error: ${{ matrix.django-version == '4.0' }}

Expand Down
20 changes: 17 additions & 3 deletions xmodule/capa/safe_exec/tests/test_safe_exec.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from six import unichr
from six.moves import range

from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.capa.safe_exec import safe_exec, update_hash
from xmodule.capa.safe_exec.remote_exec import is_codejail_rest_service_enabled

Expand Down Expand Up @@ -81,17 +82,30 @@ def test_raising_exceptions(self):


class TestSafeOrNot(unittest.TestCase): # lint-amnesty, pylint: disable=missing-class-docstring

@skip_unless_lms
def test_cant_do_something_forbidden(self):
'''
Demonstrates that running unsafe code inside the code jail
throws SafeExecException, protecting the calling process.

This test generally is skipped in CI due to its complex setup. That said, we recommend that devs who are
hacking on CodeJail or advanced CAPA in any significant way take the time to make sure it passes locally.
See either:
* in-platform setup: https://github.com/openedx/edx-platform/blob/master/xmodule/capa/safe_exec/README.rst
* remote setup (using Tutor): https://github.com/eduNEXT/tutor-contrib-codejail

Note on @skip_unless_lms:
This test can also be run in a CMS context, but that's giving us trouble in CI right now (the skip logic isn't
working). So, if you're running this locally, feel free to remove @skip_unless_lms and run it against CMS too.
'''
# Can't test for forbiddenness if CodeJail isn't configured for python.
# If in-platform codejail isn't configured...
if not jail_code.is_configured("python"):

# Can't test for forbiddenness if CodeJail rest service isn't enabled.
# Remote codejailservice must be running, see https://github.com/eduNEXT/tutor-contrib-codejail/
# ...AND if remote codejail isn't configured...
if not is_codejail_rest_service_enabled():

# ...then skip this test.
pytest.skip(reason="Local or remote codejail has to be configured and enabled to run this test.")

g = {}
Expand Down
2 changes: 2 additions & 0 deletions xmodule/capa/tests/test_xqueue_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
from xblock.fields import ScopeIds

from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.capa.xqueue_interface import XQueueInterface, XQueueService


@skip_unless_lms
class XQueueServiceTest(TestCase):
"""Test the XQueue service methods."""
def setUp(self):
Expand Down
3 changes: 3 additions & 0 deletions xmodule/tests/test_capa_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from xblock.scorable import Score

import xmodule
from openedx.core.djangolib.testing.utils import skip_unless_lms
from xmodule.capa import responsetypes
from xmodule.capa.correctmap import CorrectMap
from xmodule.capa.responsetypes import LoncapaProblemError, ResponseError, StudentInputError
Expand Down Expand Up @@ -182,6 +183,7 @@ class CapaFactoryWithFiles(CapaFactory):


@ddt.ddt
@skip_unless_lms
class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=missing-class-docstring

def setUp(self):
Expand Down Expand Up @@ -2898,6 +2900,7 @@ def test_default(self):
# ignore quotes


@skip_unless_lms
class ProblemCheckTrackingTest(unittest.TestCase):
"""
Ensure correct tracking information is included in events emitted during problem checks.
Expand Down
21 changes: 12 additions & 9 deletions xmodule/tests/test_library_tools.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""
Tests for library tools service.
Tests for library tools service (only used by CMS)
"""
from unittest.mock import patch

from opaque_keys.edx.keys import UsageKey
from openedx.core.djangolib.testing.utils import skip_unless_cms
from openedx.core.djangoapps.content_libraries import api as library_api
from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest
from openedx.core.djangoapps.xblock.api import load_block
Expand All @@ -13,6 +14,7 @@
from xmodule.modulestore.tests.utils import MixedSplitTestCase


@skip_unless_cms
class LibraryToolsServiceTest(MixedSplitTestCase):
"""
Tests for library service.
Expand Down Expand Up @@ -40,6 +42,7 @@ def test_list_available_libraries_fetch(self, mock_get_library_summaries):
assert mock_get_library_summaries.called


@skip_unless_cms
class ContentLibraryToolsTest(MixedSplitTestCase, ContentLibrariesRestApiTest):
"""
Tests for LibraryToolsService which interact with blockstore-based content libraries
Expand All @@ -63,16 +66,16 @@ def test_import_from_blockstore(self):
course = CourseFactory.create(modulestore=self.store, user_id=self.user.id)
CourseInstructorRole(course.id).add_users(self.user)
# Add Source from library block to the course
sourced_block = self.make_block("library_sourced", course, user_id=self.user.id)
lc_block = self.make_block("library_content", course, user_id=self.user.id)
Comment on lines -66 to +69
Copy link
Member Author

@kdmccormick kdmccormick Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (library_sourced being deleted in favor of library_content) happened in the source code few weeks ago. The fact that this test case was still passing (under LMS settings) means that test wasn't working right!

When run under CMS settings, though, the test case did fail (like it should have).


# Import the unit block from the library to the course
self.tools.import_from_blockstore(sourced_block, [unit_block_id])
self.tools.import_from_blockstore(lc_block, [unit_block_id])

# Verify imported block with its children
assert len(sourced_block.children) == 1
assert sourced_block.children[0].category == 'unit'
assert len(lc_block.children) == 1
assert lc_block.children[0].category == 'unit'

imported_unit_block = self.store.get_item(sourced_block.children[0])
imported_unit_block = self.store.get_item(lc_block.children[0])
assert len(imported_unit_block.children) == 1
assert imported_unit_block.children[0].category == 'html'

Expand All @@ -86,10 +89,10 @@ def test_import_from_blockstore(self):

# Check that reimporting updates the target block
self._set_library_block_olx(html_block_id, '<html><a href="/static/test.txt">Foo bar</a></html>')
self.tools.import_from_blockstore(sourced_block, [unit_block_id])
self.tools.import_from_blockstore(lc_block, [unit_block_id])

assert len(sourced_block.children) == 1
imported_unit_block = self.store.get_item(sourced_block.children[0])
assert len(lc_block.children) == 1
imported_unit_block = self.store.get_item(lc_block.children[0])
assert len(imported_unit_block.children) == 1
imported_html_block = self.store.get_item(imported_unit_block.children[0])
assert 'Hello world' not in imported_html_block.data
Expand Down
Loading