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

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Oct 18, 2023

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 block to use library_content block instead.

Before and after merging, I will update branch protection settings to account for the shard changes.

@kdmccormick kdmccormick changed the title test: run ./xmodule/ tests with CMS settings test: run ./xmodule/ tests with CMS settings too Oct 18, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/test-xmodule-with-cms branch from 3bfea38 to d409212 Compare October 18, 2023 19:42
Just like ./common/ and ./openedx/, the unit tests in ./xmodule/
validate behavior in both LMS and CMS. In order to fully test
./xmodule/, we need to run its tests in both contexts.

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.
@kdmccormick kdmccormick force-pushed the kdmccormick/test-xmodule-with-cms branch from d409212 to 8fe86a1 Compare October 18, 2023 20:38
Comment on lines -66 to +69
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)
Copy link
Member Author

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).

@kdmccormick kdmccormick marked this pull request as ready for review October 18, 2023 21:08
Copy link
Contributor

@connorhaugh connorhaugh left a comment

Choose a reason for hiding this comment

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

LGTM, pending those last LMS tests passing (they are still running.)

@kdmccormick kdmccormick merged commit 9c6e765 into master Oct 19, 2023
65 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/test-xmodule-with-cms branch October 19, 2023 14:19
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants