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

Tests for adding podcast API [WIP] #336

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SiqingYu
Copy link
Contributor

See #219 for more info.

@stefankoegl
Copy link
Member

As mentioned in the online comment, the tests never actually test the task result - just the fact that the task exists. You can look into https://docs.celeryproject.org/projects/django-celery/en/2.4/cookbook/unit-testing.html#using-a-custom-test-runner-to-test-with-celery on how to easily test task results. An alternative approach is described in https://medium.com/@erayerdin/how-to-test-celery-in-django-927438757daf

Comment on lines +197 to +207
def test_add_podcast_new(self):
"""Test add podcast API for new podcast"""
podcast_url = 'http://example.com/new-podcast.xml'
add_url = reverse('api-add-podcast')
body = {'url': podcast_url}
add_resp = self.client.post(add_url, body, content_type='application/json')
self.assertEqual(add_resp.status_code, 202)
status_url = add_resp.get('Location')
status_resp = self.client.get(status_url)
self.assertEqual(status_resp.status_code, 200)
self.assertEqual(status_resp.json().get('status'), 'pending')
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this test correctly, it only tests whether a task has been started - but not its result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefankoegl
I add tests for the Celery task result below.
Celery testing is particularly tricky. I've tried the two references you provided but could get neither pytest fixture or pytest-django working for Celery. I turn to celery.app.task.Task.apply() to execute the task ad-hoc.
The problem is that the Celery instance is the one from mygpo.celery, the same as the real one, thus accessing the production database. I think this is a potential problem. But I can't find a better solution at the moment.
Any suggestion is appreciated. 😄

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the Celery instance is the one from mygpo.celery, the same as the real one, thus accessing the production database.

When running tests, the DATABASE setting is initialized to point to the test database (see https://docs.djangoproject.com/en/dev/topics/testing/overview/#the-test-database). Therefore I don't think its possible to access the prod database through mygpo.celery from within a test. These two should be completely isolated from each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefankoegl Does the new PR for testing meet the requirement? If so, I'll move on to documentation and hopefully close the issue. 😄

@SiqingYu SiqingYu self-assigned this Jan 19, 2020
self.assertEqual(status_resp.status_code, 200)
self.assertEqual(status_resp.json().get('status'), 'pending')
job_id = status_url.split('/')[-1]
result = update_podcasts.apply(task_id=job_id)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment describing what this does exactly? That might not be obvious for somebody reading the code in the future.

self.assertEqual(status_resp.json().get('status'), 'pending')
job_id = status_url.split('/')[-1]
result = update_podcasts.apply(task_id=job_id)
self.assertTrue(result.ready())
Copy link
Member

Choose a reason for hiding this comment

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

I think we're still not testing the end result. Now the test checks whether the task has completed, but what we actually want to achieve is to have a new podcast. I think that the checks should verify that a podcast with the submitted URL has been created.

@SiqingYu SiqingYu added the user User handling (registration, session management, etc) label Jan 20, 2020
@SiqingYu SiqingYu changed the title Tests for adding podcast API Tests for adding podcast API [WIP] Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user User handling (registration, session management, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants