[Django] #16028: Consolidate default template filters' tests
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: nobody
Type: | Status: new
Cleanup/optimization | Component: Testing
Milestone: | framework
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16028>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: nobody
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Accepted | Needs tests: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by amplivibe):
* owner: nobody => amplivibe
* status: new => assigned
* ui_ux: => 0
Comment:
I'm looking at it right now
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Component: Testing framework
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by amplivibe):
Cannot do "from django.template.defaultfilters import *" in filters.py,
because there's a filter called "date", which conflicts with datetime.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Component: Testing framework
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by amplivibe):
One problem is that the tests from
[https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/templates/filters.py
templates/filters.py] are actually loaded and tested in
[https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/templates/tests.py
templates/tests.py].
[thttps://code.djangoproject.com/browser/django/trunk/tests/regressiontests/templates/tests.py
templates/tests.py] are the tests for templates. The
[https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/defaultfilters/tests.py
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: nobody
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by amplivibe):
* owner: amplivibe => nobody
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: nobody
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
Replying to [comment:4 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by Odd_Bloke):
* owner: nobody => Odd_Bloke
Comment:
I'm looking at this at the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by Odd_Bloke):
* cc: Odd_Bloke (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by Odd_Bloke):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Changes (by julien):
* owner: Odd_Bloke => julien
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by julien):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Patch needs improvement: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Replying to [comment:12 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:13>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by tobych):
* owner: julien => tobych
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:14>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by tobych):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:15>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:16>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:17>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
--------------------------------------+------------------------------------
Changes (by timo):
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:18>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:19>
Django
Reporter: julien | Owner: tobych
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by prestontimmons):
I added a pull request that merges these into
`template_tests.filter_tests`.
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:20>
Django
Reporter: julien | Owner: tobych
Component: Testing framework | Version: 1.3
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f91d7366e066dc5e1edd90c6bde438fae9fe67fb"]:
{{{
#!CommitTicketReference repository=""
revision="f91d7366e066dc5e1edd90c6bde438fae9fe67fb"
Fixed #16028 -- Moved defaultfilters tests into template_tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16028#comment:21>