Opened 13 years ago

Closed 9 years ago

#16028 closed Cleanup/optimization (fixed)

Consolidate default template filters' tests

Reported by: Julien Phalip Owned by: tobych
Component: Testing framework Version: 1.3
Severity: Normal Keywords:
Cc: Daniel Watkins Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The tests are currently split between two locations: source:django/trunk/tests/regressiontests/defaultfilters/tests.py and source:django/trunk/tests/regressiontests/templates/filters.py

For clarity it would be nice to merge the former into the latter.

Attachments (2)

defaultfilters-doc.diff (557 bytes ) - added by Jack Ha 13 years ago.
Added documentation for defaultfilters/tests.py
16028.WIP.diff (32.5 KB ) - added by Julien Phalip 13 years ago.
Work in progress

Download all attachments as: .zip

Change History (23)

comment:1 by Ramiro Morales, 13 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Jack Ha, 13 years ago

Owner: changed from nobody to Jack Ha
Status: newassigned
UI/UX: unset

I'm looking at it right now

comment:3 by Jack Ha, 13 years ago

Cannot do "from django.template.defaultfilters import *" in filters.py, because there's a filter called "date", which conflicts with datetime.

comment:4 by Jack Ha, 13 years ago

One problem is that the tests from templates/filters.py are actually loaded and tested in templates/tests.py. [thttps://code.djangoproject.com/browser/django/trunk/tests/regressiontests/templates/tests.py templates/tests.py] are the tests for templates. The defaultfilters/tests.py are tests for the defaultfilter functions themselves.

Since the files are so different, it may be better not to merge them together. Some documentation to point this out would be nice.

by Jack Ha, 13 years ago

Attachment: defaultfilters-doc.diff added

Added documentation for defaultfilters/tests.py

comment:5 by Jack Ha, 13 years ago

Owner: changed from Jack Ha to nobody
Status: assignednew

in reply to:  4 comment:6 by Julien Phalip, 13 years ago

Replying to amplivibe:

Since the files are so different, it may be better not to merge them together. Some documentation to point this out would be nice.

The two sets of tests are written in different styles, but merging is still worth doing. The idea is to convert the tests in source:django/trunk/tests/regressiontests/defaultfilters/tests.py to using the same style as in source:django/trunk/tests/regressiontests/templates/filters.py and eventually deprecate the source:django/trunk/tests/regressiontests/defaultfilters module.

comment:7 by Daniel Watkins, 13 years ago

Owner: changed from nobody to Daniel Watkins

I'm looking at this at the moment.

comment:8 by Daniel Watkins, 13 years ago

Cc: Daniel Watkins added

comment:9 by Daniel Watkins, 13 years ago

git branch available at https://github.com/OddBloke/django/tree/16028.

Thus far I've split all of the filters.py tests into separate methods, to make it more obvious what is doing what (and where changes are being made). I'm now (or maybe tomorrow) going to look at formatting the individual test cases more nicely, as they're pretty non-obvious (and long!) at the moment.

After that, I'll start looking at actually moving the defaultfilters tests across.

I'd also like to consider a way to make these proper tests (or at least to have them appear as proper tests), rather than having a slightly weird test-runner within a test-runner thing going on.

comment:10 by Julien Phalip, 13 years ago

It is good to split the tests in multiple methods, however the "get_xxxx_tests()" methods seem like an unnecessary step in this refactor, and they create a lot of boilerplate code just to execute them. It seems better to directly write actual unittest methods like in the defaultfilters tests module. Have you done any recent work on this? Otherwise I might take a stab at it.

comment:11 by Julien Phalip, 13 years ago

Owner: changed from Daniel Watkins to Julien Phalip

by Julien Phalip, 13 years ago

Attachment: 16028.WIP.diff added

Work in progress

comment:12 by Julien Phalip, 13 years ago

I have posted a work in progress (about 1/3rd of the way there) but before I go any further I'd like to get some feedback on one particular concern I have. I'm porting the tests from the defaultfilters module to match the majority of other tests in the templates.filters module. However, this way of testing tests the results as strings. For example, even though the wordcount filter returns integers, the tests check results like u'3'.

Is that a real issue? Should I keep going this way or is that a bad idea?

in reply to:  12 comment:13 by Aymeric Augustin, 13 years ago

Replying to julien:

Is that a real issue? Should I keep going this way or is that a bad idea?

I don't think it's a big deal, but if you want to be totally safe, you could add a separate test to check the type of the result of each filter that can return something other than a string.

comment:14 by tobych, 11 years ago

Owner: changed from Julien Phalip to tobych
Status: newassigned

Sarah and I here at the PyCon sprint will work on this tomorrow. Julien's just confirmed via Twitter he's not working on it.

comment:15 by tobych, 11 years ago

https://github.com/tobych/django/commits/ticket-16028-refactor-template-filter-tests

@birdsarah and I have paired at the PyCon 2013 Sprint, with help from @julien, @aaugustin and @honza, and made a few commits to the branch above. See the individual commit messages for details. To summarize, we applied @julien's original patch, rebased against master after fixing some Unicode stuff, tidied a few things and added some comments to clarify the intent we assumed when tidying up. Hopefully I'll finish this up tomorrow.

What's left with respect to the tests moved so far:

  1. filters.py has some strings in double quotes; some single. I'll change them to single quotes where appropriate. Or whatever the convention is.
  2. The old tests tested that filters returned integers as appropriate. The new tests just test against strings, so we've lost some coverage here. I'll do what I can do fix that. In discussion with @birdsarah, @julien suggests counting the tests that would need this and if there aren't too many, just add new test cases for the non-string cases. Otherwise, he suggests, consider changing test_templates() to allow non-strings to be the expected result.

Then move the remaining filter tests in defaultfilters.tests to filters.py.

comment:16 by selenamarie, 11 years ago

Has patch: set

comment:17 by Tim Graham, 11 years ago

Patch needs improvement: set

comment:18 by Tim Graham, 11 years ago

Easy pickings: unset

comment:19 by Preston Timmons, 9 years ago

Patch needs improvement: unset

comment:20 by Preston Timmons, 9 years ago

I added a pull request that merges these into template_tests.filter_tests.

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

Resolution: fixed
Status: assignedclosed

In f91d7366e066dc5e1edd90c6bde438fae9fe67fb:

Fixed #16028 -- Moved defaultfilters tests into template_tests.

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