Code

Opened 7 years ago

Closed 7 years ago

#5441 closed (fixed)

Add unit tests for django.contrib.formtools

Reported by: adrian Owned by: simeon
Component: contrib.formtools Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The django.contrib.formtools package needs unit tests. See also #5432 (the documentation ticket).

Attachments (6)

models.py (90 bytes) - added by simeon 7 years ago.
Empty models.py file to place in contrib.formtools. See comments
test_urls.py (291 bytes) - added by simeon 7 years ago.
Url file to be used only by unit tests. See comments
tests.py (3.4 KB) - added by simeon 7 years ago.
Contains unit tests for contrib.formtools.preview
5441.txt (4.1 KB) - added by simeon 7 years ago.
Diff spanning 3 files above
5441.diff (4.1 KB) - added by simeon 7 years ago.
5441.2.diff (4.3 KB) - added by simeon 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by adrian

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by tamas

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:3 Changed 7 years ago by anonymous

  • Owner changed from anonymous to tamas
  • Status changed from assigned to new

comment:4 Changed 7 years ago by anonymous

  • Owner changed from tamas to nobody

comment:5 Changed 7 years ago by simeon

  • Has patch set

comment:6 Changed 7 years ago by simeon

  • Has patch unset

Changed 7 years ago by simeon

Empty models.py file to place in contrib.formtools. See comments

Changed 7 years ago by simeon

Url file to be used only by unit tests. See comments

Changed 7 years ago by simeon

Contains unit tests for contrib.formtools.preview

comment:7 Changed 7 years ago by simeon

  • Has patch set
  • Needs tests unset
  • Owner changed from nobody to simeon
  • Status changed from new to assigned

I've attached three files:

  • models.py - this is empty but running runtests.py formtools generated an import error as the test runner attempts to import models for modules with tests.
  • test_urls.py - this contains urlpattern to map a url to the generated form to be tested. Couldn't think of any other way to do this.
  • tests.py - Uses the test Client object to test the form. Currently there are only 4 tests which verify a form is generated, verify the preview stage, and verify that the form's done() method is fired if the preview is accepted.

I'm not very experienced with python, UnitTest, or Django yet, so I'll be happy to revise if anyone has comments or requests.

Changed 7 years ago by simeon

Diff spanning 3 files above

comment:8 Changed 7 years ago by simeon

As per suggestions from gdoubleu in the sprint IRC channel: I've uploaded the output of svn diff in the django/contrib/formtools directory. I also updated the tests to use self.assertContains() to handle string discovery and status codes. I also refactored slightly to eliminate duplications.

Gdoubleu also suggested that test_urls.py may not be needed. I'm not very familiar with django internals yet and based my hack on the url dispatch process description at http://www.djangoproject.com/documentation/url_dispatch/ - if there's a better way to temporarily map urls->functions for the purposes of testing, I'd be glad to pursue it...

Changed 7 years ago by simeon

comment:9 Changed 7 years ago by simeon

Um, yes and trac wants a .diff extension to recognise a diff file. Please ignore the .txt file... It is 11pm on a Sunday :)

comment:10 Changed 7 years ago by simeon

  • Triage Stage changed from Accepted to Ready for checkin

Nobody else has commented on this, so am changing status to "Ready for checkin". I am willing to make improvements if any committers see things they don't like...

comment:11 Changed 7 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

In the future, please don't mark your own tickets ready for checkin; ask a triager or developer to review it for you.

There's a couple of problems with this patch:

  • You need to run the svn diff from the root of the Django tree. See the contributing guide for more info.
  • You need to find a better name for the unit test class ("tests" isn't very descriptive).
  • You need to fix some of your code to comply with the coding guidlines in the contributing guide.

None of these are a big deal, and a developer will happily do this for you given enough time. However, if you'd like to expedite this ticket you can help us by doing those things yourself.

comment:12 Changed 7 years ago by simeon

In the future, please don't mark your own tickets ready for checkin; ask a triager or developer to review it for you.

Will do. Is this something I should request on the Django dev list or IRC channel? Or just leave a note on the bug itself?

I will work on the patch to fix the items you mention - I should be able to update it tomorrow...

Changed 7 years ago by simeon

comment:13 Changed 7 years ago by simeon

Ok, I updated the patch (run from trunk directory). Latest version is 5441.2.diff. Class name is now "PreviewTests" - tests for other (future) parts of formtools could be added with a different class name. Methods of PreviewTests are now underscored instead of CamelCased and running pep8.py on the tests.py file results in no errors (did I really terminate a line with a semicolon? Sorry - my day job still involves lots of PHP.) If there are any other style issues I need to be aware of, please let me know.

I'm also opening a bug on the documentation to suggest that the "Contributing" section mention pep8.py. Maybe this is common knowledge to experienced devs but it helped me catch some issues I wouldn't have seen (trailing whitespace, semi-colons)...

comment:14 Changed 7 years ago by simeon

  • Patch needs improvement unset

Am I allowed to uncheck "patch needs improvement"? Or should I let jacob uncheck it since he set it in the first place. In any event, it's been a couple weeks since the last update to the bug so I'm just trying to get it off my mental todo list...

comment:15 Changed 7 years ago by gwilson

  • Triage Stage changed from Accepted to Ready for checkin

patch was updated, bumping up.

comment:16 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [6734]) Fixed #5441 -- Added tests for django.contrib.formtools. Thanks, simeon.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.