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.

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

Tests for adding podcast API [WIP] #336

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

Tests for adding podcast API [WIP] #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f65b25
Tests for adding new podcasts API.
SiqingYu Jan 17, 2020
e2095ef
Correct Location header of response from adding podcast API for newly…
SiqingYu Jan 17, 2020
2899ce1
Test result of adding new podcast API.
SiqingYu Jan 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions mygpo/api/advanced/directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ def episode_info(request):
@allowed_methods(['POST'])
@cors_origin()
def add_podcast(request):
# TODO what if the url doesn't have a valid podcast?
url = normalize_feed_url(json.loads(request.body.decode('utf-8')).get('url', ''))

# 404 before we query for url, because query would complain
Expand All @@ -114,9 +113,9 @@ def add_podcast(request):
res = update_podcasts.delay([url])
response = HttpResponse(status=202)
job_status_path = reverse(
'api-add-podcast-status', kwargs={"job_id": res.task_id}
'api-add-podcast-status', kwargs={'job_id': res.task_id}
)
response['Location'] = f'{job_status_path}?url={url}'
response['Location'] = f'{job_status_path}'
return response


Expand Down
25 changes: 25 additions & 0 deletions mygpo/api/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from mygpo.history.models import EpisodeHistoryEntry
from mygpo.test import create_auth_string
from mygpo.utils import get_timestamp
from mygpo.data.tasks import update_podcasts


class AdvancedAPITests(unittest.TestCase):
Expand Down Expand Up @@ -185,6 +186,30 @@ def test_episode_info(self):

self.assertEqual(resp.status_code, 200)

def test_add_podcast_existed(self):
"""Test add podcast API for existed podcast"""
url = reverse('api-add-podcast')
body = {'url': self.podcast.url}
location = reverse('api-podcast-info') + '?url=' + self.podcast.url
add_resp = self.client.post(url, body, content_type='application/json')
self.assertEqual(add_resp.status_code, 302)
self.assertEqual(add_resp.get('Location'), location)

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')
Comment on lines +198 to +208
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. 😄

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.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.



class EpisodeActionTests(TestCase):
def setUp(self):
Expand Down
4 changes: 3 additions & 1 deletion mygpo/api/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@
advanced.directory.episode_info,
name='api-episode-info',
),
path('api/2/podcasts/create', advanced.directory.add_podcast),
path(
'api/2/podcasts/create', advanced.directory.add_podcast, name='api-add-podcast'
),
path(
'api/2/task/<uuid:job_id>',
advanced.directory.add_podcast_status,
Expand Down
2 changes: 2 additions & 0 deletions mygpo/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,3 +401,5 @@ def get_intOrNone(name, default):

except (ImportError, ValueError):
pass

TEST_RUNNER = 'djcelery.contrib.test_runner.CeleryTestSuiteRunner'