Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #24782 -- Added assertFormValid and assertFormNotValid methods. #5595

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@jvzammit
Copy link

jvzammit commented Nov 7, 2015

Adds two functions under django.test.testcases.SimpleTestCase as instructed in this #4645 that has so far been closed:

  • assertFormValid
  • assertFormNotValid

The two functions are inspired by this comment by @mjtamlyn .

Unit tests have also been added for each function covering the possible use cases.

Since this is my first PR to an open-source project ever I'm appreciative of feedback you will (should) have.

P.S. This PR has started off by applying user delgiudices work (1st commit).
Will squash commits once review feedback is applied.

@charettes charettes changed the title Ticket 24782 -- Add TestCase.assertFormValid Fixed #24782 -- Added a assertFormValid and assertFormInvalid methods. Nov 7, 2015

@charettes charettes changed the title Fixed #24782 -- Added a assertFormValid and assertFormInvalid methods. Fixed #24782 -- Added assertFormValid and assertFormInvalid methods. Nov 7, 2015

@charettes

View changes

django/test/testcases.py Outdated
allow_other_errors=True, context_name='form',
status_code=200, msg=None):
raise NotImplementedError

This comment has been minimized.

Copy link
@charettes

charettes Nov 7, 2015

Member

I guess these methods could be part of another commit?

@charettes

View changes

tests/test_utils/tests.py Outdated
name = forms.CharField(label='Name', max_length=4)
age = forms.IntegerField(label='Age')

form_cls = TestForm

This comment has been minimized.

Copy link
@charettes

charettes Nov 7, 2015

Member

No need to define another attribute. The form class should be accessible as self.TestForm.

@charettes

View changes

tests/test_utils/tests.py Outdated
allow_other_errors=False)

self.assertEqual(
cm.exception.message, u"""FormNotValid failed. Error(s):

This comment has been minimized.

Copy link
@charettes

charettes Nov 7, 2015

Member

No need for the u prefix, we're already importing unicode_literals.

@charettes

View changes

tests/test_utils/tests.py Outdated

self.assertEqual(
cm.exception.message,
u'''Form validation failed. Error(s):

This comment has been minimized.

Copy link
@charettes

charettes Nov 7, 2015

Member

Ditto.

@charettes

View changes

tests/test_utils/tests.py Outdated

self.assertEqual(
cm.exception.message,
u'FormNotValid failed. Error(s):\n- Form is valid\n')

This comment has been minimized.

Copy link
@charettes

charettes Nov 7, 2015

Member

Ditto.

@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Nov 7, 2015

@charettes Review feedback applied, thanks for the feedback! I'll squash commits once I get your "go ahead".

@timgraham

View changes

tests/test_utils/tests.py Outdated
@@ -4,13 +4,13 @@
import unittest
import warnings

from django import forms

This comment has been minimized.

Copy link
@timgraham

timgraham Nov 19, 2015

Member

Please keep cleanups like this in a separate commit or pull request to avoid blaming those changes to a commit which contains new features.

This comment has been minimized.

Copy link
@jvzammit

jvzammit Nov 20, 2015

Author

@timgraham Thank you for reviewing this. I don't understand what is meant "Please keep cleanups like this in a separate commit or pull request". I know it affects previous code but the alternative would have been to import Form, CharField and IntegerField from django.forms individually. Can you kindly instruct and what should be done exactly? Please bear with me as this is my first-ever PR.

This comment has been minimized.

Copy link
@timgraham

timgraham Nov 20, 2015

Member

Create a separate pull request or commit in this pull request that precedes the changes for the ticket which makes the import change.

@timgraham

View changes

tests/test_utils/tests.py Outdated
'age': None,
})

with self.assertRaises(AssertionError) as cm:

This comment has been minimized.

Copy link
@timgraham

timgraham Nov 19, 2015

Member

use self.assertRaisesMessage()

@timgraham

View changes

docs/topics/testing/tools.txt Outdated
@@ -1381,6 +1381,24 @@ your test suite.
``errors`` is an error string, or a list of error strings, that are
expected as a result of form validation.

.. method:: SimpleTestCase.assertFormValid(form, msg=None)

Asserts that a form is valid. if form is not valid. Displays clean output with

This comment has been minimized.

Copy link
@timgraham

timgraham Nov 19, 2015

Member

Please use periods after all sentences and add .. versionadded:: 1.10 annotations as described in the patch review checklist. Also a mention in the 1.10 release notes is needed. Be sure to wrap all documentation at 79 characters.

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Nov 19, 2015

Please uncheck "Patch needs improvement" on the ticket when you update the patch. It's fine to squash commits and use a commit message that matches the pull request title. Thanks!

Merge pull request #1 from django/master
Merge latest Django into fork.
@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Nov 21, 2015

@timgraham Ready to commit. However, what should I do about the Py3 test failures? Thanks.

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Nov 22, 2015

Use self.assertRaisesMessage()

@jvzammit

View changes

tests/test_utils/tests.py Outdated
@@ -10,7 +10,7 @@
from django.core.files.storage import default_storage
from django.core.urlresolvers import NoReverseMatch, reverse
from django.db import connection, router
from django.forms import EmailField, IntegerField
from django.forms import CharField, EmailField, Form, IntegerField

This comment has been minimized.

Copy link
@jvzammit

jvzammit Nov 22, 2015

Author

@timgraham I just added more individual imports here rather than causing (and requiring) a "refactoring" PR or commit as you had suggested. That doesn't cause touching of previously-existing code. Wrt all other changes, all improvements you suggested were applied. Thanks again.

@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Nov 22, 2015

Btw @timgraham I'm trying to unset "Patch needs improvement" from the ticket but I forgot how to edit tickets on Trac. Sorry about that.

@timgraham

View changes

docs/topics/testing/tools.txt Outdated

``expected_errors`` is a list of (field, code) errors that are expected on the form.

``allow_other_errors`` is a boolean that when set (by default) allows other errors not within ``expected_errors`` to be raised by the form.

This comment has been minimized.

Copy link
@timgraham

timgraham Dec 24, 2015

Member

Docs should be wrapped at 79 characters.

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Dec 24, 2015

Please add a mention in the 1.10 release notes too.
About editing the ticket, you need to be logged in to Trac to edit -- could that be issue?

Merge pull request #2 from django/master
Merge latest Django into fork.

@jvzammit jvzammit changed the title Fixed #24782 -- Added assertFormValid and assertFormInvalid methods. Fixed #24782 -- Added assertFormValid and assertFormNotValid methods. Dec 26, 2015

@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Dec 26, 2015

@timgraham Added mention in release docs, and wrapped docs lines (except for function signature, all current function signatures in docs appear to go over that 79-chars line wrapping rule). Rebased over the latest Django and pushed again.

Also unchecked "patch needs improvement" in Trac once I logged in.

I still can't figure out why the method reference in release notes is failing. I.e. the method references should be found. Any help with that?

Thanks again! Sorry this is taking so long.

Merge pull request #3 from django/master
Merge latest Django into fork.
@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Jan 26, 2016

Rebased on latest master to fix conflict, pushed again. Failed again at docs, asked this question on StackOverflow someone suggests a resolution to this. CC' @timgraham

@@ -301,7 +301,7 @@ Templates
Tests
~~~~~

* ...
* Added methods :meth:`~django.test.testcases.SimpleTestCase.assertFormValid` and :meth:`~django.test.testcases.SimpleTestCase.assertFormNotValid`.

This comment has been minimized.

Copy link
@timgraham

timgraham Jan 26, 2016

Member

omit ".testcases" -- the documentation references use the convenience import path to SimpleTestCase.

This comment has been minimized.

Copy link
@jvzammit

jvzammit Jan 26, 2016

Author

Thanks @timgraham that eliminates the warning locally. Committed the change, rebased to squash last commit and pushed again. Thanks!

@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Jan 26, 2016

@timgraham Is this ready to merge (finally)?

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Jan 26, 2016

I'll give it another review when I have a chance. Can you try to rebase your branch so that the "Merge" commits don't appear here?

@@ -301,7 +301,7 @@ Templates
Tests
~~~~~

* ...
* Added methods :meth:`~django.test.SimpleTestCase.assertFormValid` and :meth:`~django.test.SimpleTestCase.assertFormNotValid`.

This comment has been minimized.

Copy link
@timgraham

timgraham Jan 26, 2016

Member

docs should be wrapped at 79 characters

@jvzammit

This comment has been minimized.

Copy link
Author

jvzammit commented Jan 26, 2016

@timgraham Closing this PR in favour of #6046 .

@jvzammit jvzammit closed this Jan 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.