Skip to content

test:enable the disabled tests in test_capa_block.py - #32557

Closed
raju249 wants to merge 1 commit into
openedx:masterfrom
raju249:31695-update-disabled-test
Closed

test:enable the disabled tests in test_capa_block.py #32557
raju249 wants to merge 1 commit into
openedx:masterfrom
raju249:31695-update-disabled-test

Conversation

@raju249

@raju249 raju249 commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Description

In #3872, masking is disabled, so instead of removing these tests, we can enable them but change the assertion to test actual answer responses. Also, the rescore function name was changed from rescore_problem to rescore.

Resolves #31695

Supporting information

PR where masking was disabled - #3872

Testing instructions

xmodule/tests/test_capa_block.py should pass as expected with all tests.

Deadline

None

Other information

NA

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 23, 2023
@openedx-webhooks

openedx-webhooks commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Thanks for the pull request, @raju249! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@raju249

raju249 commented Jun 23, 2023

Copy link
Copy Markdown
Author

@openedx/cla-problems - I have signed CLA that I received in the email yesterday.

@raju249 raju249 changed the title Enable the disabled tests until masking is disabled globally Enable the disabled tests until masking is disabled Jun 23, 2023
@raju249 raju249 changed the title Enable the disabled tests until masking is disabled Enable the disabled tests until masking is disabled Jun 23, 2023
@raju249 raju249 changed the title Enable the disabled tests until masking is disabled Enable the disabled tests until masking is disabled Jun 23, 2023
@raju249

raju249 commented Jun 23, 2023

Copy link
Copy Markdown
Author

@feanil - I have signed the CLA, awaiting on the team to sign it so that the check passes.

Whom can I reach out for help?

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 23, 2023
@e0d

e0d commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

@raju249 same as #32559 we need a signed CLA and updated commit messages.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 26, 2023
@raju249
raju249 force-pushed the 31695-update-disabled-test branch from 54cb955 to 8569695 Compare June 26, 2023 14:52
@raju249 raju249 changed the title Enable the disabled tests until masking is disabled test:enable the disabled tests in test_capa_block.py Jun 26, 2023
@raju249

raju249 commented Jun 26, 2023

Copy link
Copy Markdown
Author

Thanks @e0d

I have fixed the commit messages and the CLA should be resolved soon!

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 26, 2023
@raju249

raju249 commented Jun 27, 2023

Copy link
Copy Markdown
Author

@e0d - CLA is complete. Can you please help me with a review?

Thanks!

@e0d e0d changed the title test:enable the disabled tests in test_capa_block.py test:enable the disabled tests in test_capa_block.py Jun 27, 2023
@raju249 raju249 changed the title test:enable the disabled tests in test_capa_block.py test:enable the disabled tests in test_capa_block.py Jun 27, 2023
@raju249

raju249 commented Jun 27, 2023

Copy link
Copy Markdown
Author

Hmm. I am not sure why this still complains about CLA 🤔

@raju249

raju249 commented Jun 28, 2023

Copy link
Copy Markdown
Author

@e0d - Whom can I check about the CLA issue? I have the confirmation mail that CLA is signed by both parties. This is also an issue in #32559

@e0d e0d changed the title test:enable the disabled tests in test_capa_block.py test:enable the disabled tests in test_capa_block.py. Jun 28, 2023
@e0d e0d changed the title test:enable the disabled tests in test_capa_block.py. test:enable the disabled tests in test_capa_block.py Jun 28, 2023
@e0d

e0d commented Jun 28, 2023

Copy link
Copy Markdown
Contributor

@raju249 the CLA is sorted out. We're getting there :) Looks like there a new commitlint issue, standard.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 28, 2023
Rajendra Kadam
test: enable the disabled tests in test_capa_block.py
03d6ad2 
Since in openedx#3872, we disabled masking, this PR enables the test
with actual output from the choice.

This seems to be better than removing them until masking is enabled.
@raju249
raju249 force-pushed the 31695-update-disabled-test branch from 80abc67 to 03d6ad2 Compare June 28, 2023 09:05
@raju249

raju249 commented Jun 28, 2023

Copy link
Copy Markdown
Author

@e0d - Can you re-run the pipeline, please?

I had followed the standard while rebasing the other day. 🤔

Lets re-run and check.

@raju249

raju249 commented Jun 30, 2023

Copy link
Copy Markdown
Author

@e0d - The pipeline is 🟢.

Can you please help with a review and merge this PR?

@e0d
e0d requested review from feanil and nedbat June 30, 2023 10:56
@e0d

e0d commented Jun 30, 2023

Copy link
Copy Markdown
Contributor

Thanks for getting this PR to 🟢 , 🎆 .

I've requested that @feanil and @nedbat have a look as they were involved in the original issue that motivated this work.

@mphilbrick211

mphilbrick211 commented Jul 18, 2023

Copy link
Copy Markdown

Thanks for getting this PR to 🟢 , 🎆 .

I've requested that @feanil and @nedbat have a look as they were involved in the original issue that motivated this work.

Hi @feanil and @nedbat! Just following up on this.

@feanil

feanil commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

@mphilbrick211 we're waiting on the author to respond on the related issue #31695

They'll need to rework this PR to match the changes requested there.

  • Delete the tests and references to masking since the feature has not existed in 7 years.

@mphilbrick211 mphilbrick211 added the inactive PR author has been unresponsive for several months label Sep 12, 2023
@mphilbrick211

mphilbrick211 commented Oct 3, 2023

Copy link
Copy Markdown

Hi @raju249! I'm going to close this PR for now. If you would like to pursue in the future, we can re-open. Thanks!

@openedx-webhooks

openedx-webhooks commented Oct 3, 2023

Copy link
Copy Markdown

@raju249 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks

openedx-webhooks commented Oct 3, 2023

Copy link
Copy Markdown

@raju249 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@mphilbrick211 mphilbrick211 added closed inactivity PR was closed because the author abandoned it and removed inactive PR author has been unresponsive for several months labels Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nedbat nedbat Awaiting requested review from nedbat
@feanil feanil Awaiting requested review from feanil

Assignees

No one assigned

Labels

closed inactivity PR was closed because the author abandoned it open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"masking temporarily disabled" >8 years ago: should we unskip or delete these tests?

5 participants

@raju249 @openedx-webhooks @e0d @mphilbrick211 @feanil