-
Notifications
You must be signed in to change notification settings - Fork 342
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
Adds tests for invalid template variables #222
Conversation
e7b1e95
to
8d90b0f
Compare
I honestly don't know why the tests for 1.8 are failing. Does anyone have a clue to why the marker does not work? |
This changed in 1.8: https://docs.djangoproject.com/en/1.8/ref/templates/upgrading/ It should only be deprecated, but maybe there's a bug with that, so that e.g. |
The problem is likely that you would need to change the |
@blueyed thanks, I'll look into that and tie it up. |
9ce0665
to
e73eade
Compare
@blueyed I fixed the django 18 issue and added documentation. Have fun! |
"""Fixture that fails for invalid variables in templates. | ||
|
||
This fixture will fail each test that uses django template rendering | ||
should at template contain an invalid template variable. |
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.
s/at/a/
Very nice, I've left a few comments. |
@blueyed thanks, I updated my commit. |
This would be very useful! I think this should be opt-in (maybe as a pytest.ini setting+marker for local overrides?). Just enabling it for everyone will probably break a lot of existing tests. Maybe this ( |
@pelme I don't really think that Django has any interest in that patch. They actually have an exception that they always catch. Regarding opt-in/out: I think this is an exceptable change for a minor version. And yes, some people will have to fix some templates upgrading. But only if there templates are wrong ;) If you want me to make this an opt-in feature, let me know, I'll update my code and doc. |
This is similar to Therefore I think that this feature must be opt-in, but it is certainly something I'd like to use myself. |
@pelme sure, I'll update my PR |
@codingjoe |
@codingjoe Very nice! What about naming the option a bit less generic than |
Sure, will do. Might actually be a good Idea as I'm thinking about writing more template tests like a py.test wrapper for tidy-html5. |
@blueyed all done. |
# Configure FAIL_INVALID_TEMPLATE_VARS | ||
itv = (options.itv or | ||
early_config.getini(INVALID_TEMPLATE_VARS_ENV) or | ||
os.environ.get(INVALID_TEMPLATE_VARS_ENV)) |
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.
This should be changed to prefer the environment variable before the config, see #199.
|
@blueyed I swapped the config. |
|
|
|
@blueyed oh yes, sorry, I fixed that. |
I think it might be good if it would use "1" for the environment variable, and allow "0" to disable it. |
Django catches all `VariableDoesNotExist` exceptions to replace them in templates with a modifiable string that you can define in your settings. Sadly that doesn't allow you to find them in unit tests. `_fail_for_invalid_template_variable` sets the setting `TEMPLATE_STRING_IF_INVALID` to a custom class that not only fails the current test but prints a pretty message including the template name. This behavior can be used with the new `--test-templates` command line option. A new marker allows disabling this behavior, eg: @pytest.mark.ignore_template_errors def test_something(): pass This marker sets the setting to None, if you want it to be a string, you can use the `settings` fixture to set it to your desired value.
@blueyed I hope that does fixes the issue. Please feel free to fix any typos while merging. I'm dyslexic and going into a 20th-ish review cycle is somewhat disencouraging. |
|
Make it possible to fail tests for invalid template variables. Thanks to Johannes Hoppe for the patch and Daniel Hahler for review.
@codingjoe Thanks a lot for fixing all the comments and sticking with it. This is a great feature that I will use myself. Sorry for the slow response, but now it is merged and will be included in the next version (which probably will be out during/after this weekend). |
@codingjoe |
Changes Unknown when pulling 954081f on codingjoe:master into * on pytest-dev:master*. |
else: | ||
msg = "Undefined template variable '%s'" % var | ||
if self.fail: | ||
pytest.fail(msg, pytrace=False) |
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.
@codingjoe
Do you remember why you used pytrace=False
here?
I think it is very valuable to have a traceback in case of errors, isn't it?
(it might need to be massaged a bit to make it shorter though).
(via #470)
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.
Nope, I don't. I think back then there trace didn't give you much information, because the template engine worked differently. That was before you could switch engines, if I remember correctly.
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.
@codingjoe
Thanks for your feedback, enabled it in 760fb18 (#470).
Django catches all
VariableDoesNotExist
exceptions to replacethem in templates with a modifiable string that you can define in
your settings.
Sadly that doesn't allow you to find them in unit tests.
_fail_for_invalid_template_variable
sets the settingTEMPLATE_STRING_IF_INVALID
to a custom class that not only failsthe current test but prints a pretty message including the template
name.
A new marker allows disabling this behavior, eg:
This marker sets the setting to None, if you want it to be a string,
you can use the
settings
fixture to set it to your desired value.