Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add tests using default DRF classes #490

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

Merged
sliverc merged 8 commits into django-json-api:master from Alig1493:test-default-drf-serializers
Oct 15, 2018

Conversation

Copy link
Contributor

@Alig1493 Alig1493 commented Oct 9, 2018 •
edited by sliverc
Loading

Fixes #283

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • changelog entry added to CHANGELOG.md
  • author name in AUTHORS

Copy link
Contributor Author

Alig1493 commented Oct 9, 2018

@sliverc Here's the initial progress.

@Alig1493 Alig1493 force-pushed the test-default-drf-serializers branch 2 times, most recently from 1e71502 to b42b20b Compare October 9, 2018 10:22
Copy link
Member

sliverc commented Oct 11, 2018

Thanks @Alig1493 This looks like a good start.

The test you are covering here is based on rendering. I think it will also be important to test parsing with DRF serializers resp. the full request/response cycle. This could be something like test_model_viewsets.py but with DRF classes.

Copy link
Contributor Author

Alright I'll go take a look.

Copy link
Contributor Author

@sliverc I had some issues regarding testing the response of the default drf serializer urls:
The test_model_viewsets.py file tested urls that called the viewsets which had json-api serializers.
Does this mean I'll be making my own custom viewsets and register it with the urls as new endpoints and test those new endpoints?

I took the approach of making new serializers for the Blog model using the default serializer and adding it as a viewset in urls. I didn't make new files for these and added the code to the existing code base. Should I push into the PR for you to check?

Thanks.

Copy link
Member

sliverc commented Oct 11, 2018

@Alig1493 yep you will add custom DRF viewsets for this to work. I am fine when they are in the same file as it just a variant of the other tests.

So 👍 to push

Copy link
Contributor Author

@sliverc pushed.

Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Great. It's shaping up nicely. See my comments. Also it would still be great to have a test which either does a post or patch on the DRF view (I guess the same view could be used).



# DRF default serializer to test default DRF functionalities
class BlogDRFSerializer(serializers.ModelSerializer):
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be derived from drf_serilazers.ModelSerializer as well? Also best move the comment into the class a block comment.

assert result['data']['attributes']['json-field'] == {'json-key': 'JsonValue'}


class TestDRFBlogViewSet(TestBlogViewSet):
Copy link
Member

Choose a reason for hiding this comment

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

as all other tests are written in pytest in this module I would write these tests the same way. This way we do not have a dependency on TestBlogViewSet (which basically only creates an entry during setup phase anyway).

'meta': {'apiDocs': '/docs/api/blogs'}
}
got = resp.json()
print(got)
Copy link
Member

Choose a reason for hiding this comment

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

left over print statement? I guess should be removed.

@@ -0,0 +1,99 @@
import json

from django.urls import reverse
Copy link
Member

Choose a reason for hiding this comment

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

@pytest.mark.django_db
def test_get_object_gives_correct_blog(client, blog, entry):

url = reverse('drf-entry-blog', kwargs={'entry_pk': entry.id})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sliverc I have used reverse here.

settings.JSON_API_FORMAT_KEYS = 'dasherize'
rendered = render_dummy_test_serialized_view(DummyTestViewSet)

result = json.loads(rendered.decode())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sliverc as for json here's the usage

Copy link
Contributor Author

@sliverc apologies for taking so long to get back to you I became occupied with something else for some time. One thing that I was wondering is that if it would be possible for you to guide me as to how to make put/patch requests using modelviewsets. I've mostly used generic class based views, although that's no excuse. My initial attempt in trying to patch the drf-entry-blog url endpoint of mine keeps giving me method not allowed error.

Copy link
Contributor Author

@sliverc Update: I've figured out how to patch using routers and model viewsets. Hopefully you'll be able to review it soon. My remaining question is that when posting data should I be only creating a Blog object, or will I have to link the blog object to the entry object to which it's related.

@Alig1493 Alig1493 force-pushed the test-default-drf-serializers branch 2 times, most recently from 315ab6c to ebc5e4f Compare October 14, 2018 11:21
Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Create test looks good. Let's leave it as is for now we might add relationships creation later one when there is a need.

Some small comments remain. Would also be great if you could add yourself to AUTHORS.



@pytest.fixture
def blog():
Copy link
Member

Choose a reason for hiding this comment

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

This is actually not needed as this is automatically done with pytest-factoryboy where this factory is registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's my bad on not seeing the registered factories.

blog = Blog.objects.filter(name=name)

# check if blog exists in database
assert blog.exists()
Copy link
Member

Choose a reason for hiding this comment

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

I guess this asserts is not needed as the next asserts checks already whether it exists by counting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

assert blog.count() == 1

# get created blog from database
blog = blog[0]
Copy link
Member

Choose a reason for hiding this comment

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

blog.first() might be more elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OF COURSE. Completely missed that train of thought.

Copy link
Contributor Author

@sliverc I have made changes as per your review. Please have a look.

Copy link
Contributor Author

@sliverc WIth your permission I've also included my name to authors. Put that in a seperate commit if that's okay.

Copy link
Member

sliverc commented Oct 15, 2018

Looks good.

It needs to be rebased to current master and CI isort issue https://travis-ci.org/django-json-api/django-rest-framework-json-api/jobs/441354640 fixed and the in should be ready.

Copy link
Contributor Author

@sliverc I can do the rebase, but I'll need more insight on the isort issue. It's been there since my first push!

Copy link
Contributor Author

Isort is a package that sorts imports alphabetically. So I just have to fix my order of imports?

@Alig1493 Alig1493 force-pushed the test-default-drf-serializers branch 2 times, most recently from 136f7c1 to b7b6606 Compare October 15, 2018 09:05
Copy link
Contributor Author

@sliverc Travis CI seems to be stuck. Could you please give more insight on this issue? :s

Copy link
Member

sliverc commented Oct 15, 2018

@Alig1493 Usually pushing again to the branch helps. I guess you can rebase to master and resolve the conflicts and push again.

@Alig1493 Alig1493 force-pushed the test-default-drf-serializers branch from b7b6606 to 5780a68 Compare October 15, 2018 09:34
Copy link
Contributor Author

@sliverc it seems all's well for now.

@sliverc sliverc changed the title (削除) Added rendering tests using default drf serializers (削除ここまで) (追記) Add tests using default DRF classes (追記ここまで) Oct 15, 2018
@sliverc sliverc merged commit bda8f63 into django-json-api:master Oct 15, 2018
Copy link
Member

sliverc commented Oct 15, 2018

Great. Thanks for your contribution. Merged.

In case you want to extend your tests to address #298 that would be great. Feel free.

Copy link
Contributor Author

I'd love to extend my help if possible. I'll take a look and ask if there's any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sliverc sliverc sliverc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants

AltStyle によって変換されたページ (->オリジナル) /