Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#23768 closed Cleanup/optimization (fixed)

Rewrite template_tag.tests.TemplateTests as individual test cases

Reported by: Preston Timmons Owned by: Aymeric Augustin
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (20)

comment:1 by Yamila, 10 years ago

Triage Stage: UnreviewedAccepted

Agree. May be labourious as there are lots of tests, but tests should be smaller and isolated.

comment:2 by Yamila, 10 years ago

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.

comment:3 by Preston Timmons, 10 years ago

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.

comment:4 by Preston Timmons, 10 years ago

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.

comment:5 by Preston Timmons, 9 years ago

I added a pull request here:

https://github.com/django/django/pull/3631

comment:6 by Preston Timmons, 9 years ago

Has patch: set

comment:7 by Aymeric Augustin, 9 years ago

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?

comment:8 by Preston Timmons, 9 years ago

I can work on the remainder of the tests. I'll post an update when I'm done.

comment:9 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:10 by Preston Timmons, 9 years ago

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.

comment:11 by Berker Peksag, 9 years ago

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.

comment:12 by Berker Peksag, 9 years ago

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.

comment:13 by Aymeric Augustin, 9 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

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.

comment:14 by Preston Timmons, 9 years ago

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.

comment:15 by Preston Timmons, 9 years ago

Patch needs improvement: unset

comment:16 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In b872134bfc14f6322bd1e4b0a08bf5bfd2c43a52:

Fixed #23768 -- Rewrote template tests as unit tests.

comment:17 by Berker Peksag, 9 years ago

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.

comment:18 by Berker Peksag, 9 years ago

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

comment:19 by Baptiste Mispelon <bmispelon@…>, 9 years ago

In 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

comment:20 by Tim Graham <timograham@…>, 9 years ago

In 16f26defa7510707742a15aa89cae56f11d14c3f:

Converted recently refactored templates tests to SimpleTestCase.

These test methods don't need DB setup/teardown.

Refs #23768 and b872134b.

Note: See TracTickets for help on using tickets.
Back to Top