pinax / pinax-ratings

a ratings app for Django
MIT License
90 stars 28 forks source link

Add Template tag tests #47

Closed kasulani closed 6 years ago

kasulani commented 6 years ago
grahamu commented 6 years ago

@kasulani I'm somewhat confused... you posted "Done @grahamu as requested" on all my requests three days ago, but there is no new code since ten days ago. Did you forget to push your commits?

grahamu commented 6 years ago

Aha, now I realize your PR is not complete @kasulani. I'll wait to hear from you.

kasulani commented 6 years ago

@grahamu I have pushed some commits to fix the issues you pointed out in the review

grahamu commented 6 years ago

@kasulani your tests are greatly improved! You've correctly addressed all but one of my requests.

The only remaining issue is test_ratings_tag, where you are testing the wrong assertion.

Upon review, my earlier comment is partially incorrect:

This test passes, but I do not believe it should. Write tests to prove an assertion, not to pass. In this case the test docstring is wrong and the "ratings" ttag works improperly.

So the first two sentences are correct, but the third, asserting "ratings" ttag code is wrong, is wrong. I'm now thinking that "ratings" ttag is correct, it just isn't properly tested.

First correct the docstring by examining the "ratings" ttag code and deciding what it should return in this situation. Then write assertions proving the expected value is returned.

This is still true, and this is where you missed my direction. Examine the "ratings" ttag code and see what is getting returned. The way I see it, this ttag is returning a QuerySet of Ratings, and the test should verify the queryset. The template rendered in this test produces "2" because {% ratings object %} shows the queryset contents, and this Rating is "2". However, this does not prove the correct queryset is returned, just that there is a "2" in the rendered template.

I believe properly constructed assertions will fail and the code should be corrected.

Pretty sure I'm wrong here... pretty sure "ratings" ttag code is OK.

Hint: verifying queryset equality can be tricky. I typically verify same number of items in each QuerySet, then convert both QuerySets to sets and verify set equality.

Yep. This test should not render a template. Instead examine and verify the return value from "ratings" ttag, similar to what you've done with test_user_rating_js_tag. Don't render a template at all. I typically use assertSetEqual for queryset equality (when duplicates are not expected) because it ignores object order:

ct = ContentType.objects.get_for_model(self.benz)
output = rating(<args>)
expected = Rating.objects.filter(content_type=ct, object_id=self.benz.pk)
self.assertSetEqual(set(output), set(expected))

So one more update and I think this will be ready for merge. Thanks so much for your efforts @kasulani!

grahamu commented 6 years ago

BTW @kasulani you are making great progress, and significantly improving test coverage for pinax-ratings, as seen in this coverage report:

screen shot 2018-03-14 at 2 34 46 pm