Skip to content

Allow tests for Extracted & Builtin LTI XBlock #36020

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.

Sign up for GitHub

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

Jump to bottom
Draft
ttqureshi wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Allow tests for Extracted & Builtin LTI XBlock #36020

ttqureshi wants to merge 9 commits into from
+153 −34

Conversation

@ttqureshi
Copy link
Member

@ttqureshi ttqureshi commented Dec 12, 2024 •
edited
Loading

Test the LTI XBlock in both built-in and extracted modes to keep them in sync.

Related to: #36281

@ttqureshi ttqureshi marked this pull request as draft December 12, 2024 08:31
@ttqureshi ttqureshi marked this pull request as ready for review December 12, 2024 08:37
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from ba481cd to 8343717 Compare December 12, 2024 08:40
@ttqureshi ttqureshi marked this pull request as draft December 12, 2024 09:21
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 4 times, most recently from d132ef3 to eaf2743 Compare February 17, 2025 10:19
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 4 times, most recently from 8a886fd to 48be2e4 Compare February 21, 2025 05:57
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from cf6d9cc to 995261a Compare March 17, 2025 09:35
@ttqureshi
Copy link
Member Author

ttqureshi commented Apr 3, 2025 •
edited
Loading

@kdmccormick
test_number_mongo_calls this test case is failing with the test params expecting 37 mongo calls as in 2nd, 3rd and 4th test case parameters.

With the flag (USE_EXTRACTED_LTI_BLOCK) set to True, 36 calls are made and thus assertion fails. I have tried to figure out the reason but couldn't get any clue.

I'm sharing the output diff here. Please take a look, and guide me what could be the possible way-outs.

cc: @feanil @farhan

@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from f5560d2 to efc6cb7 Compare April 4, 2025 13:25
@farhan
Copy link
Contributor

farhan commented Apr 5, 2025

@ttqureshi

test_number_mongo_calls this test case is failing with the test params expecting 37 mongo calls as in 2nd, 3rd and 4th test case parameters.

  1. We are not adding the EmptyDataRawMixin in the extracted XBlock.
  2. EmptyDataRawMixin has data String field which is missing in the extracted XBlock
  3. This 1 less field is causing 1 less mongo DB call. So either
    a. add this data field for initial extraction to make exact code
    b. or decrease 1 count in test case

Note: I don't have technical depth of the mongo calls working, it needs to be explored

@ttqureshi
Copy link
Member Author

ttqureshi commented Apr 7, 2025 •
edited
Loading

@farhan
Thanks for giving the pointers.

  1. We are not adding the EmptyDataRawMixin in the extracted XBlock.
  2. EmptyDataRawMixin has data String field which is missing in the extracted XBlock
  3. This 1 less field is causing 1 less mongo DB call. So either
    a. add this data field for initial extraction to make exact code
    b. or decrease 1 count in test case

Seems like this is causing the issue. Let me try things around first, and IMO decreasing the count by 1 in the test case (inside if condition) sounds more reasonable than adding extra data field that is not required anywhere in the extracted LTIBlock.

@ttqureshi ttqureshi mentioned this pull request Apr 8, 2025
Merged
8 tasks
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 3 times, most recently from 373aa3e to 58f8e54 Compare April 10, 2025 13:38
@kdmccormick kdmccormick self-requested a review April 10, 2025 17:14
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 2 times, most recently from 324c681 to 9c4c049 Compare April 15, 2025 09:46
@ttqureshi ttqureshi mentioned this pull request Apr 15, 2025
Closed
2 tasks
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from 9c4c049 to b5f2551 Compare April 16, 2025 13:44
@ttqureshi ttqureshi mentioned this pull request May 19, 2025
Closed
@kdmccormick kdmccormick self-assigned this Aug 1, 2025
@kdmccormick kdmccormick removed their request for review August 14, 2025 17:03
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from 4164531 to b2ffa96 Compare September 16, 2025 08:36
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 2 times, most recently from dc60155 to b17047f Compare September 16, 2025 13:41
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Sep 16, 2025
@ttqureshi ttqureshi self-assigned this Sep 16, 2025
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 3 times, most recently from a44d7b8 to 5f4531c Compare September 19, 2025 08:11
@ttqureshi ttqureshi changed the title Test PR | Enable Extracted LTI Block Allow tests for Extracted & Builtin LTI XBlock Sep 22, 2025
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch 2 times, most recently from 3b23988 to bb41e8d Compare September 22, 2025 07:53
salman2013
salman2013 requested changes Sep 23, 2025
View reviewed changes
lms/djangoapps/courseware/tests/test_lti_integration.py


@override_settings(USE_EXTRACTED_LTI_BLOCK=True)
class TestLTIBlockListingExtracted(_TestLTIBlockListing):
Copy link
Contributor

@salman2013 salman2013 Sep 23, 2025

Choose a reason for hiding this comment

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

_TestLTIBlockListingBase could be better name to represent the base class.

xmodule/tests/test_lti20_unit.py

class LTI20RESTResultServiceTest(unittest.TestCase):
from xmodule.lti_2_util import LTIError as BuiltInLTIError
from xblocks_contrib.lti.lti_2_util import LTIError as ExtractedLTIError
Copy link
Contributor

@salman2013 salman2013 Sep 23, 2025

Choose a reason for hiding this comment

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

I would suggest to have a reset_class method in lti_2_util.py class which return the builtin or extracted LTIError just like we are doing in lti_block.py

edx-platform/xmodule/lti_block.py

Line 998 in bb41e8d

def reset_class():

i think in this way if we have to filter for LTIError we would have one single import and one place from where it would come as per flag setting.

@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from bb41e8d to 0f2c8c6 Compare September 25, 2025 07:34
@mphilbrick211
Copy link

mphilbrick211 commented Nov 13, 2025

Hi @ttqureshi! Is this pull request still in progress?

@mphilbrick211 mphilbrick211 added the inactive PR author has been unresponsive for several months label Nov 13, 2025
@ttqureshi
Copy link
Member Author

ttqureshi commented Nov 14, 2025

@mphilbrick211
YES, it's a WIP. It is currently blocked on this. This is the new story for the #XBlock/823.

@mphilbrick211 mphilbrick211 added blocked by other work PR cannot be finished until other work is complete and removed inactive PR author has been unresponsive for several months labels Dec 12, 2025
@mphilbrick211 mphilbrick211 moved this from Waiting on Author to Blocked in Contributions Dec 12, 2025
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from 0f2c8c6 to 4193626 Compare January 1, 2026 13:35
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from 4193626 to c6d0b64 Compare January 1, 2026 13:36
@ttqureshi ttqureshi force-pushed the ttqureshi/test-lti-block branch from c6d0b64 to 80773ab Compare January 1, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@salman2013 salman2013 salman2013 requested changes

@farhan farhan Awaiting requested review from farhan farhan will be requested when the pull request is marked ready for review farhan is a code owner

@irtazaakram irtazaakram Awaiting requested review from irtazaakram irtazaakram will be requested when the pull request is marked ready for review irtazaakram is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@kdmccormick kdmccormick

@ttqureshi ttqureshi

Labels

blocked by other work PR cannot be finished until other work is complete open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Status: Blocked

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow tests for Extracted & BuiltIn LTI XBlock

6 participants

@ttqureshi @farhan @openedx-webhooks @mphilbrick211 @salman2013 @kdmccormick