Skip to content

Commit

Permalink
test: run ./xmodule/ tests with CMS settings (#33534)
Browse files Browse the repository at this point in the history
Currently, ./xmodule/ unit tests are only run with LMS settings. However,
./common/ and ./xmodule/ are run twice: once with LMS settings and once with
CMS settings.

Just like ./common/ and ./openedx/, the unit tests in ./xmodule/ validate
behavior in both LMS and CMS. So, order to fully test ./xmodule/, we should to
run its tests with CMS settings too.

This will enable us to better validate certain LibraryContentBlocks behaviors
being touched by #33263 which can't
be expressed under LMS settings.

Also in this commit:

* refactor: rename the shards to be clear whether they're running under LMS or CMS
* docs: correct comments regarding conditions under which codejail's
   test_cant_do_something_forbidden is skipped.
* test: update a unit test which was using the now-deleted library_sourced block to use
   library_content block instead.
  • Loading branch information
kdmccormick authored Oct 19, 2023
1 parent 98e8bf8 commit 9c6e765
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 33 deletions.
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)

# 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

0 comments on commit 9c6e765

Please sign in to comment.