[Django] #23768: Rewrite template_tag.tests.TemplateTests as individual test cases
Django
--------------------------------------+--------------------
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
There are some downsides to the current format of the template tests:
* It's impossible to use `--failfast`
* All errors are lumped into a single failure. When working on the
template code, a small problem can result in hundreds of errors worth of
output.
* When there is an error, the output isn't helpful since the test cases
themselves are difficult to read.
I think it would be an improvement if each template tag had it's own test
case with each line as a separate test method.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yamila-moreno):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Agree. May be labourious as there are lots of tests, but tests should be
smaller and isolated.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:1>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by yamila-moreno):
I've introduced an error in the test, and run the tests like follows:
PYTHONPATH=..:$PYTHONPATH ./runtests.py --settings=test_sqlite
template_tests --failfast
and it works properly.
But anyway, the errors are shown dirtier than expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:2>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
If you introduce multiple errors to TemplateTests and run `./runtests.py
--settings=test_sqlite template_tests.tests.TemplateTests --failfast`, it
will not end on the first failure since all the tests are treated as a
single test method.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:3>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I've added a proof-of-concept for this here:
https://github.com/prestontimmons/django/compare/ticket-23768
This is a rewrite of most of the "basic-syntax-*" tests into a
BasicSyntaxTests class.
It maintains the current behavior of the test runner, which runs each test
6 times with combinations of TEMPLATE_DEBUG, TEMPLATE_STRING_IF_INVALID,
and the cached loader, with the following advantages:
* Individual tests can be run easily, like `./runtests.py
--settings=test_sqlite
template_tests.test_clean.BasicSyntaxTests.test_text`.
* The --failfast option works correctly. `./runtests.py
--settings=test_sqlite template_tests.test_clean.BasicSyntaxTests
--failfast`.
* Readability is improved.
Is this approach acceptable? Porting existings tests over to it is easy.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:4>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I added a pull request here:
https://github.com/django/django/pull/3631
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:5>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:6>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The tests are still hard to figure out because there's a lot of code
factorization but I think it's an improvement.
Would you like me to merge the pull request now? Or do you want to port
the remainder of the tests first?
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:7>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I can work on the remainder of the tests. I'll post an update when I'm
done.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:8>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:9>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by prestontimmons):
I updated my pull request to include the remainder of the template tests.
I'm going to do a final check through everything before unchecking patch
needs improvement.
For ease of auditing, I kept all the test methods named the same as
before, i.e. include01, include02, etc. I think it would be good to add
meaningful names in a separate patch.
The new suite adds 640 tests, including the additional test for the setup
function. The previous suite had 641 tests, two of which weren't really
tests themselves: `include 05` and `include-cycle`. I verified this with
this script:
https://gist.github.com/prestontimmons/01ed288aaa2083afa2d4
I didn't update the filter tests yet. I think these should also be done in
a separate patch.
I have a couple specific questions:
1) There are two tests that seem to have wrong comments:
* filter-syntax25
* if-tag-shortcircuit02
In both cases, the tests do the opposite of what the comment says. For
`filter-syntax25` I believe the comment is wrong. For `if-tag-
shortcircuit02`, I think the test is wrong.
Does anyone else agree?
2) I added a warnings filter for the url tag tests to the setup function.
Is this done right? I'm not too familiar with how warnings work.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:10>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by berkerpeksag):
> 2) I added a warnings filter for the url tag tests to the setup
function. Is this done right? I'm not too familiar with how warnings work.
Code LGTM. Just one nitpick: You could use
`warnings.simplefilter("ignore", ...)` if you don't planning to pass a
`message` parameter to `warnings.filterwarnings`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:11>
Django
Reporter: prestontimmons | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by berkerpeksag):
> For filter-syntax25 I believe the comment is wrong.
+1.
> For if-tag-shortcircuit02, I think the test is wrong.
I think the comment is a bit misleading here.
The original comment was:
> When the if clause is shortcircuiting correctly, the is_bad() function
shouldn't be evaluated,
Copied from dc3b2a0fdf2a69864c5508a9c49aca43acac9bb2.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:12>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* status: new => assigned
* owner: nobody => aaugustin
Comment:
I'll take a look at your questions and merge the patch as soon as
possible. I don't want it to become stale.
Please add yourself in the AUTHORS file (in alphabetical order) if you
aren't already there.
Only dealing with tags is good for now. The patch is large enough already.
Thank you very much.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:13>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by prestontimmons):
The patch has been updated with some good feedback by Tim Graham. I think
it's good to go now.
Some things to do after merge are:
* Add meaningful names to test methods
* Update filter tests
* Decide if each test needs to run with `TEMPLATE_DEBUG` and
`TEMPLATE_STRING_IF_INVALID`. Tim mentioned it may be more appropriate to
just add separate test cases where these are used.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:14>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:15>
Django
Reporter: prestontimmons | Owner: aaugustin
Cleanup/optimization | Version: master
Component: Template system | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b872134bfc14f6322bd1e4b0a08bf5bfd2c43a52"]:
{{{
#!CommitTicketReference repository=""
revision="b872134bfc14f6322bd1e4b0a08bf5bfd2c43a52"
Fixed #23768 -- Rewrote template tests as unit tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:16>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Template system | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by berkerpeksag):
Hi,
I'm trying to review https://github.com/django/django/pull/3660 and I have
a question about this patch:
Is there a reason why `setup` invokes a test method twice?
https://github.com/django/django/blob/master/tests/template_tests/syntax_tests/utils.py#L58
This will make harder to test deprecation warnings correctly.
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:17>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Template system | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by berkerpeksag):
I've found a bug in the `setup` decorator.
{{{
$ ./tests/runtests.py
template_tests.syntax_tests.test_if.IfTagTests.test_if_tag_eq06
Testing against Django installed in '/home/berker/projects/django/django'
Traceback (most recent call last):
File "./tests/runtests.py", line 410, in <module>
options.reverse, options.modules)
File "./tests/runtests.py", line 236, in django_tests
test_labels or get_installed(), extra_tests=extra_tests)
File "/home/berker/projects/django/django/test/runner.py", line 154, in
run_tests
suite = self.build_suite(test_labels, extra_tests)
File "/home/berker/projects/django/django/test/runner.py", line 74, in
build_suite
tests = self.test_loader.loadTestsFromName(label)
File "/usr/lib/python2.7/unittest/loader.py", line 109, in
loadTestsFromName
return self.suiteClass([parent(obj.__name__)])
File "/usr/lib/python2.7/unittest/case.py", line 191, in __init__
(self.__class__, methodName))
ValueError: no such test method in <class
'template_tests.syntax_tests.test_if.IfTagTests'>: inner
}}}
Here is a PR to fix this: https://github.com/django/django/pull/3671
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:18>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Template system | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Baptiste Mispelon <bmispelon@…>):
In [changeset:"adacbd64a062662f54d6e91dc4e460eff96b5dd5"]:
{{{
#!CommitTicketReference repository=""
revision="adacbd64a062662f54d6e91dc4e460eff96b5dd5"
Fixed "no such test method" error in template_tests.
Without this patch, you couldn't run an individual test
case in template_tests.
Refs #23768
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:19>
Django
Reporter: prestontimmons | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Template system | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"16f26defa7510707742a15aa89cae56f11d14c3f"]:
{{{
#!CommitTicketReference repository=""
revision="16f26defa7510707742a15aa89cae56f11d14c3f"
Converted recently refactored templates tests to SimpleTestCase.
These test methods don't need DB setup/teardown.
Refs #23768 and b872134b.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23768#comment:20>