From e63a7a492059afa7e6b9c662fe7055b6561bdbaf Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 28 Apr 2020 17:31:45 -0400 Subject: [PATCH] Revert "remove tests causing flakiness (#23827)" (#23830) This reverts commit be1aaa00ff994b66c6b00281b17a254a51771c23. --- .../certificates/tests/test_models.py | 14 + .../programs/tasks/v1/tests/test_tasks.py | 274 ++++++++++++++++++ .../djangoapps/programs/tests/test_signals.py | 61 ++++ 3 files changed, 349 insertions(+) diff --git a/lms/djangoapps/certificates/tests/test_models.py b/lms/djangoapps/certificates/tests/test_models.py index 3d26cebd39..5a43e5b0ca 100644 --- a/lms/djangoapps/certificates/tests/test_models.py +++ b/lms/djangoapps/certificates/tests/test_models.py @@ -9,6 +9,7 @@ from django.core.exceptions import ValidationError from django.core.files.uploadedfile import SimpleUploadedFile from django.test import TestCase from django.test.utils import override_settings +from mock import patch from opaque_keys.edx.locator import CourseKey, CourseLocator from path import Path as path @@ -357,3 +358,16 @@ class CertificateInvalidationTest(SharedModuleStoreTestCase): self.assertFalse( CertificateInvalidation.has_certificate_invalidation(self.user, self.course_id) ) + + @patch('openedx.core.djangoapps.programs.tasks.v1.tasks.revoke_program_certificates.delay') + @patch( + 'openedx.core.djangoapps.credentials.models.CredentialsApiConfig.is_learner_issuance_enabled', + return_value=True, + ) + def test_revoke_program_certificates(self, mock_issuance, mock_revoke_task): # pylint: disable=unused-argument + """ Verify that `revoke_program_certificates` is invoked upon invalidation. """ + # Invalidate user certificate + self.certificate.invalidate() + + self.assertEqual(mock_revoke_task.call_count, 1) + self.assertEqual(mock_revoke_task.call_args[0], (self.user.username, self.course_id)) diff --git a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py index 230dc13862..ef3c80c6fb 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py @@ -591,3 +591,277 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): tasks.award_course_certificate.delay(self.student.username, str(self.certificate.course_id)).get() self.assertFalse(mock_post_course_certificate.called) + + +@skip_unless_lms +class RevokeProgramCertificateTestCase(TestCase): + """ + Test the revoke_program_certificate function + """ + + @httpretty.activate + def test_revoke_program_certificate(self): + """ + Ensure the correct API call gets made + """ + test_username = 'test-username' + test_client = EdxRestApiClient('http://test-server', jwt='test-token') + + httpretty.register_uri( + httpretty.POST, + 'http://test-server/credentials/', + ) + + tasks.revoke_program_certificate(test_client, test_username, 123) + + expected_body = { + 'username': test_username, + 'status': 'revoked', + 'credential': { + 'program_uuid': 123, + 'type': tasks.PROGRAM_CERTIFICATE, + } + } + last_request_body = httpretty.last_request().body.decode('utf-8') + self.assertEqual(json.loads(last_request_body), expected_body) + + +@skip_unless_lms +@ddt.ddt +@mock.patch(TASKS_MODULE + '.revoke_program_certificate') +@mock.patch(TASKS_MODULE + '.get_certified_programs') +@mock.patch(TASKS_MODULE + '.get_inverted_programs') +@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') +class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase): + """ + Tests for the 'revoke_program_certificates' celery task. + """ + + def setUp(self): + super(RevokeProgramCertificatesTestCase, self).setUp() + + self.student = UserFactory.create(username='test-student') + self.course_key = 'course-v1:testX+test101+2T2020' + self.site = SiteFactory() + self.site_configuration = SiteConfigurationFactory(site=self.site) + ApplicationFactory.create(name='credentials') + UserFactory.create(username=settings.CREDENTIALS_SERVICE_USERNAME) + self.create_credentials_config() + + self.inverted_programs = {self.course_key: [{'uuid': 1}, {'uuid': 2}]} + + def test_inverted_programs( + self, + mock_get_inverted_programs, + mock_get_certified_programs, # pylint: disable=unused-argument + mock_revoke_program_certificate, # pylint: disable=unused-argument + ): + """ + Checks that the Programs API is used correctly to determine completed + programs. + """ + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + mock_get_inverted_programs.assert_any_call(self.student) + + def test_revokinging_certificate( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Checks that the Credentials API is used to revoke certificates for + the proper programs. + """ + expected_program_uuid = 1 + mock_get_inverted_programs.return_value = { + self.course_key: [{'uuid': expected_program_uuid}] + } + mock_get_certified_programs.return_value = [expected_program_uuid] + + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + + call_args, _ = mock_revoke_program_certificate.call_args + self.assertEqual(call_args[1], self.student.username) + self.assertEqual(call_args[2], expected_program_uuid) + + @ddt.data( + ('credentials', 'enable_learner_issuance'), + ) + @ddt.unpack + def test_retry_if_config_disabled( + self, + disabled_config_type, + disabled_config_attribute, + *mock_helpers + ): + """ + Checks that the task is aborted if any relevant api configs are + disabled. + """ + getattr(self, 'create_{}_config'.format(disabled_config_type))(**{disabled_config_attribute: False}) + with mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: + with self.assertRaises(MaxRetriesExceededError): + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + self.assertTrue(mock_warning.called) + for mock_helper in mock_helpers: + self.assertFalse(mock_helper.called) + + def test_abort_if_invalid_username(self, *mock_helpers): + """ + Checks that the task will be aborted and not retried if the username + passed was not found, and that an exception is logged. + """ + with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: + tasks.revoke_program_certificates.delay('nonexistent-username', self.course_key).get() + self.assertTrue(mock_exception.called) + for mock_helper in mock_helpers: + self.assertFalse(mock_helper.called) + + def test_abort_if_no_program( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Checks that the task will be aborted without further action if course is + not part of any program. + """ + mock_get_inverted_programs.return_value = {} + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + self.assertTrue(mock_get_inverted_programs.called) + self.assertFalse(mock_get_certified_programs.called) + self.assertFalse(mock_revoke_program_certificate.called) + + def _make_side_effect(self, side_effects): + """ + DRY helper. Returns a side effect function for use with mocks that + will be called multiple times, permitting Exceptions to be raised + (or not) in a specified order. + + See Also: + http://www.voidspace.org.uk/python/mock/examples.html#multiple-calls-with-different-effects + http://www.voidspace.org.uk/python/mock/mock.html#mock.Mock.side_effect + + """ + + def side_effect(*_a): + if side_effects: + exc = side_effects.pop(0) + if exc: + raise exc + return mock.DEFAULT + + return side_effect + + def test_continue_revoking_certs_if_error( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Checks that a single failure to revoke one of several certificates + does not cause the entire task to fail. Also ensures that + successfully revoked certs are logged as INFO and warning is logged + for failed requests if there are retries available. + """ + mock_get_inverted_programs.return_value = self.inverted_programs + mock_get_certified_programs.side_effect = [[1], [1, 2]] + mock_revoke_program_certificate.side_effect = self._make_side_effect([Exception('boom'), None]) + + with mock.patch(TASKS_MODULE + '.LOGGER.info') as mock_info, \ + mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + + self.assertEqual(mock_revoke_program_certificate.call_count, 3) + mock_warning.assert_called_once_with( + u'Failed to revoke certificate for program {uuid} of user {username}.'.format( + uuid=1, + username=self.student.username) + ) + mock_info.assert_any_call(mock.ANY, 1, self.student.username) + mock_info.assert_any_call(mock.ANY, 2, self.student.username) + + def test_retry_on_credentials_api_errors( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Ensures that any otherwise-unhandled errors that arise while trying + to get existing program credentials (e.g. network issues or other + transient API errors) will cause the task to be failed and queued for + retry. + """ + mock_get_inverted_programs.return_value = self.inverted_programs + mock_get_certified_programs.return_value = [1] + mock_get_certified_programs.side_effect = self._make_side_effect([Exception('boom'), None]) + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + self.assertEqual(mock_get_certified_programs.call_count, 2) + self.assertEqual(mock_revoke_program_certificate.call_count, 1) + + def test_retry_on_credentials_api_429_error( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Verify that a 429 error causes the task to fail and then retry. + """ + exception = exceptions.HttpClientError() + exception.response = mock.Mock(status_code=429) + mock_get_inverted_programs.return_value = self.inverted_programs + mock_get_certified_programs.return_value = [1, 2] + mock_revoke_program_certificate.side_effect = self._make_side_effect( + [exception, None] + ) + + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + + self.assertEqual(mock_revoke_program_certificate.call_count, 3) + + def test_no_retry_on_credentials_api_404_error( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Verify that a 404 error causes the task to fail but there is no retry. + """ + exception = exceptions.HttpNotFoundError() + exception.response = mock.Mock(status_code=404) + mock_get_inverted_programs.return_value = self.inverted_programs + mock_get_certified_programs.return_value = [1, 2] + mock_revoke_program_certificate.side_effect = self._make_side_effect( + [exception, None] + ) + + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + + self.assertEqual(mock_revoke_program_certificate.call_count, 2) + + def test_no_retry_on_credentials_api_4XX_error( + self, + mock_get_inverted_programs, + mock_get_certified_programs, + mock_revoke_program_certificate, + ): + """ + Verify that other 4XX errors cause task to fail but there is no retry. + """ + exception = exceptions.HttpClientError() + exception.response = mock.Mock(status_code=418) + mock_get_inverted_programs.return_value = self.inverted_programs + mock_get_certified_programs.return_value = [1, 2] + mock_revoke_program_certificate.side_effect = self._make_side_effect( + [exception, None] + ) + + tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + + self.assertEqual(mock_revoke_program_certificate.call_count, 2) diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index b47921e8e3..dfb2817a19 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -163,3 +163,64 @@ class CertChangedReceiverTest(TestCase): site_config.save() handle_course_cert_changed(**self.signal_kwargs) self.assertFalse(mock_task.called) + + +# The credentials app isn't installed for the CMS. +@skip_unless_lms +@mock.patch('openedx.core.djangoapps.programs.tasks.v1.tasks.revoke_program_certificates.delay') +@mock.patch( + 'openedx.core.djangoapps.credentials.models.CredentialsApiConfig.is_learner_issuance_enabled', + new_callable=mock.PropertyMock, + return_value=False, +) +class CertRevokedReceiverTest(TestCase): + """ + Tests for the `handle_course_cert_revoked` signal handler function. + """ + + @property + def signal_kwargs(self): + """ + DRY helper. + """ + return dict( + sender=self.__class__, + user=UserFactory.create(username=TEST_USERNAME), + course_key=TEST_COURSE_KEY, + mode='test-mode', + status='test-status', + ) + + def test_signal_received(self, mock_is_learner_issuance_enabled, mock_task): # pylint: disable=unused-argument + """ + Ensures the receiver function is invoked when COURSE_CERT_REVOKED is + sent. + + Suboptimal: because we cannot mock the receiver function itself (due + to the way django signals work), we mock a configuration call that is + known to take place inside the function. + """ + COURSE_CERT_REVOKED.send(**self.signal_kwargs) + self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1) + + def test_programs_disabled(self, mock_is_learner_issuance_enabled, mock_task): + """ + Ensures that the receiver function does nothing when the credentials API + configuration is not enabled. + """ + handle_course_cert_revoked(**self.signal_kwargs) + self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1) + self.assertEqual(mock_task.call_count, 0) + + def test_programs_enabled(self, mock_is_learner_issuance_enabled, mock_task): + """ + Ensures that the receiver function invokes the expected celery task + when the credentials API configuration is enabled. + """ + mock_is_learner_issuance_enabled.return_value = True + + handle_course_cert_revoked(**self.signal_kwargs) + + self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1) + self.assertEqual(mock_task.call_count, 1) + self.assertEqual(mock_task.call_args[0], (TEST_USERNAME, TEST_COURSE_KEY))