-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Switch to SharedModuleStoreTestCase in the 'contentstore' app where possible. #11552
Conversation
/cc @ormsbee |
@doctoryes: Do you have a moment to review some test optimizations? @tobz is converting some of our suite from |
def setUp(self): | ||
super(TestTaskExecution, self).setUp() | ||
SignalHandler.course_published.disconnect(listen_for_course_publish) | ||
SignalHandler.library_updated.disconnect(listen_for_library_update) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to disconnect signals here, since nothing is happening in the setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, does this setUp()
need to exist at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It seems like they could be in the class setup. I'll try it and see what happens.
Minor issue about signal disconnecting and whether that particular |
That's how it's done - and all tests still pass, so seems like no test-to-test interference. 👍 |
Switch to SharedModuleStoreTestCase in the 'contentstore' app where possible.
This is switching from ModuleStoreTestCase, to SharedModuleStoreTestCase, where possible. This is only for the 'contentstore' Django application. This helps speed up some tests where there's a lot of write-once-read-many for modulestore-based data.