-
Notifications
You must be signed in to change notification settings - Fork 346
Add initial approach to put Django's asserts into pytest's namespace. Issue #97 (was: Pull Request #144) #232
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
Conversation
Conflicts: pytest_django/lazy_django.py pytest_django/plugin.py
You haven't addressed the "PEP8 names would be better" comments from @roganov and @pelme (#144 (comment)), do you? Maybe both variants could be provided (Django's original name and a PEP8 variant)? But that would clutter the namespace probably and cause confusion.
(via http://stackoverflow.com/a/28774760/15690) What do you think? |
@blueyed |
@roganov
(or |
To be honest, I believed the Also, I haven't taken the time to figure out how to extract the Where can I read up on how py.test implements the |
I don't know if pytest's assertion magic is really useful here, e.g. with The methods can be extracted from Django using the code from #97 (comment) (via Update: it's good that I could imagine making it dynamic, but IIRC there has been some argument against that. |
def test_sanity(admin_client): | ||
from pytest_django import assertContains | ||
|
||
response = admin_client.get('/admin-required/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is necessarily slow.
A faster functionality should be tested here.
Re CamelCase to snake_case: The inflection library does that job (too). >>> import inflection
>>> inflection.underscore('assertHTMLNotEqual')
'assert_html_not_equal' |
Re pytest's assertion magic: What do we want Django test code to look like? Rather than assertJSONNotEqual(response.content, {'status': 'success'})
assertInHTML('foo bar baz') or assert_json_not_equal(response.content, {'status': 'success'})
assert_in_html('foo bar baz') personally, I'd prefer assert not response.content == {'status': 'success'}
assert 'foo bar baz' in html or something similar. |
maybe a compromise (so you don't need to override magic methods to make assert work) would be to have functions returning booleans, so you can do: assert json_cmp(respons.content, {'status': 'success'})
assert in_html('foo bar baz') The problem with your assert is that I think json compare and inhtml both do more than simple comparisons (in the json case I think it even parses json from a string). |
|
Who decides what future code should look like in pytest-django? Maybe we should talk about this first. |
I think currently it's available as I'd like to hear @pelme's opinion on this, of course. |
Personally, I like @santagada's proposal: from pytest.django import json_equal # Thin wrapper for Django's TestCase.assertJSONEqual
assert json_equal(response.content, {'foo': 1234}) ... which uses Django's helpful assertion error message from assertJSONEqual. This style also allows negation However, it is not really possible to have both nice error messages and possibility to do negation without special support from pytest (I think). We had some discussions at FOSDEM about having hooks to provide customized assertion outputs from single boolean results in pytest itself. IIRC we did not really come to a conclusion on how exactly it should work or what is actually needed. /cc @hpk42 @RonnyPfannschmidt @flub @bubenkoff I can see how something like this would be useful outside of pytest-django. Personally, I have a bunch of private/project specific assertion helpers that would be nicer by having something like this. I will open a pytest issue on this (hopefully today) with more details and continue the discussion over there. If this turns out to be a bad idea/not practical, I'd say |
@bittner Thanks a lot for pushing this forward! How would |
As already admitted earlier I'm not a py.test expert, and I haven't taken the time to look under the hood of py.test. Yes, it's true that constructs like And when I choose py.test for more readable tests I cannot accept The boolean function approach is probably a step in the right direction. If anyone finds a better way one day we can always deprecate those functions to get rid of them again (tedious but possible). -- But, thinking further, can we reuse Django's boilerplate functions for that (in an automated fashion, inheritance or generation), or do we have to re-implement them from scratch? |
I find the the standard assert with standard Python operators sufficient most times. However, in Python it is not possible to check for equality of HTML fragments directly and a lot of other assertions that Django can do (check for template file usage, count database queries etc). I think we must fully reuse Django's assertion helpers by wrapping them in some way such that you can use them with pytest in a nice and readable way, and not have to subclass from Django's TestCase. Django's assertion helpers are sometimes quite complex and it would not be feasible to maintain our own versions of them within pytest-django. |
I don't have too much time to put into this myself, but will be happy to review/help out wherever I can! Just drop @pelme in a comment and I'll respond as quick as possible :) |
@simodalla Simone, I've noticed that you had planned to hold a talk about "Pytest & Django are really good friends!!" at PyCon 7 in Florence. Maybe you're interested to bring this PR forward (with me)? |
+1 for an easy way to do django TestCase asserts in py.test |
'assertInHTML', 'assertJSONEqual', 'assertJSONNotEqual', | ||
'assertNotContains', 'assertNumQueries', 'assertQuerysetEqual', | ||
'assertRaisesMessage', 'assertRedirects', 'assertTemplateNotUsed', | ||
'assertTemplateUsed', 'assertXMLEqual', 'assertXMLNotEqual'), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the sets are equal, maybe it could be moved into some constant?!
I.e. one for Django 1.8 and than use it for 1.9 and 1.10 also?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the duplication here is not exactly accidental, I think it's save to say that it's merely coincidental. There is most probably no deep underlying principle behind what functions are shared between several versions and which aren't.
Thus, extracting the common function names to eliminate duplication might make this configuration less clear and maintainable instead of more so. (cf. Beware the Share by Udi Dahan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment, they can be formed dynamically rather than using any duplication in the file or from django itself.
'assertNotContains', 'assertNumQueries', 'assertQuerysetEqual', | ||
'assertRaisesMessage', 'assertRedirects', 'assertTemplateNotUsed', | ||
'assertTemplateUsed', 'assertXMLEqual', 'assertXMLNotEqual'), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be extracted dyanmically with e.g
In [3]: {attr for attr in SimpleTestCase.__dict__ if attr.startswith('assert')}
Out[3]:
{'assertContains',
'assertFieldOutput',
'assertFormError',
'assertFormsetError',
'assertHTMLEqual',
'assertHTMLNotEqual',
'assertInHTML',
'assertJSONEqual',
'assertJSONNotEqual',
'assertNotContains',
'assertRaisesMessage',
'assertRedirects',
'assertTemplateNotUsed',
'assertTemplateUsed',
'assertXMLEqual',
'assertXMLNotEqual'}
This works because a class's __dict__
contains the names defined in it, but not looking up its MRO.
You probably need to look at TestCase
and TransactionTestCase
too to get them all, didn't check that.
this one is badly timed, as i want to continue to propose to kill the pytest_namespace hook, as it requires quite a mess wrt pluginmanager usage and early initialization |
@RonnyPfannschmidt what do you propose instead? Just use the pytest_django namespace? |
@pelme for example, basically any place where you can just work with module globals and function declarations |
What are the steps needed to help get this through? These are super helpful commands. Definitely don't want to touch the database if I don't have to, which is where I love what Pytest is doing. |
any news on this issue? |
Expanded namespace of pytest Reference: pytest-dev/pytest-django#97 pytest-dev/pytest-django#232
@RonnyPfannschmidt Can you comment on the current state? What would make most sense to do to get the "combination" of plain |
to have a module inside of pytest_django one can import for the assertion helpers |
I think this is better on the Django side, eg providing these comparison functions as importable from |
@graingert Was also thinking about that, but then even if Django would provide pure functions, then they wouldn't be called in the Pytesty way like But then I don't think that extending how pytest does the comparison is possible. I only found this about extending the report of the comparison. I'm starting to get this feeling that some of the stuff here, like HTML comparisons, etc. should be made in it's own library (maybe not even pytest-specific), cause it's pretty framework-agnostic. But I'm just a random person from the Internet. |
def assertion_func(*args, **kwargs): | ||
from django.test import TestCase as DjangoTestCase | ||
|
||
getattr(DjangoTestCase('run'), name)(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ('run')
do in this case? Just trying to understand why it's necessary
JFI: there is a new take on this in #709. |
Let's follow up there. |
I've only addressed the straight-forward things from the code review feedback in PR #144 for now.
Original issue: #97