519110 - no unit tests for adding domains to ispdb

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect, P1)

Description

[Jay Schmidek]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/532.1 (KHTML, like Gecko) Chrome/4.0.213.1 Safari/532.1
Build Identifier: 

wish there were unit tests

Reproducible: Always

Comment 1

[Jay Schmidek]

Attachment: adds missing tests for adding domains to ispdb

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 403115 [details] [diff] [review]
adds missing tests for adding domains to ispdb

>+++ b/.hgignore	Sun Sep 27 10:06:29 2009 -0600
>+++ b/.hgpatchinfo/settings_py_fix.dep	Sun Sep 27 10:06:29 2009 -0600
>+++ b/.hgpatchinfo/unit_tests.dep	Sun Sep 27 10:06:29 2009 -0600

I think we don't want to check these in to subversion.
I don't know how you'll manage to get a diff without them, but it's worth figuring out for the other students.
(At worst, you might be able to do 'hg diff *', to skip the ".hg*" directories.)

>+++ b/tests/tests.py	Sun Sep 27 10:06:29 2009 -0600

I don't see this file or directory anywhere, which means that I can't apply this patch to the source tree as it is.  If you base one of your patches on top of someone else's patch, you should mention that in the comments to the attachment.  Or better, in the description.

>@@ -1,3 +1,5 @@
>+# -*- coding: latin-1 -*-
>+

I'm a much bigger fan of utf-8, because it'll let you test international strings much easier.
(Also, I believe python's default source encoding is utf-8, so you should be able to just remove those two lines.)
Of course, you'll need to set your editor to save stuff in utf-8, but that's probably a good idea anyways.  ;)

>@@ -5,30 +7,74 @@
>+    def test_multiple_ask(self):
>+        self.client.post(reverse('ispdb_add'),{'asking_or_adding':'asking','form-TOTAL_FORMS':'1','form-INITIAL_FORMS':'1','form-0-name':'ty.com'})
>+        self.client.post(reverse('ispdb_add'),{'asking_or_adding':'asking','form-TOTAL_FORMS':'1','form-INITIAL_FORMS':'1','form-0-name':'ty.com'})

These lines are really long.  We try to stay within 80 characters.  What about:
        self.client.post(reverse('ispdb_add'),{'asking_or_adding':'asking',
                                               'form-TOTAL_FORMS':'1',
                                               'form-INITIAL_FORMS':'1',
                                               'form-0-name':'ty.com'})
?

>+        
  ^^^^^^^^
Please remove the trailing spaces here (and the other places they occur).

>+    def test_add_internationalization(self):
>+        name = u'Iñtërnâtiônàlizætiøn'

Heh.  This looked way nicer in the diff.
I wonder if you're actually using utf-8 already?

>+        assert isinstance(domain,models.Domain)
>+        nose.tools.assert_equal(domain.name,name)

I might consider using 'from nose.tools import *' to let you shorten the asserts.
(Usually importing * is frowned upon, but just this once...  ;)

>+    @nose.tools.raises(models.Domain.DoesNotExist)
>+    def test_add_long_port(self):
>+        port = '80000000000000000000000000000000000000000000000000000000000000000000'
>+        self.client.post(reverse('ispdb_add'), {'asking_or_adding':'adding','form-TOTAL_FORMS':'1','form-INITIAL_FORMS':'1','form-0-name':'test','display_name':'test3','display_short_name':'test3','incoming_type':'imap','incoming_hostname':'foo','incoming_port':port,'incoming_socket_type':'None','incoming_authentication':'plain','outgoing_hostname':'bar','outgoing_port':'334','outgoing_socket_type':'TLS','outgoing_username_form':'%25EMAILLOCALPART%25','outgoing_authentication':'plain','settings_page_url':'google.com','settings_page_language':'en','confirmations':'0','problems':'0'})
>+        model = models.Domain.objects.get(name="test")

Does the self.client.post return an error?  It should, and I think we should check for that.

>+    @nose.tools.raises(models.Domain.DoesNotExist)
>+    def test_add_negative_port(self):

Ditto here.

>+    @nose.tools.raises(Exception)
>+    def test_add_letters_port(self):

What exception does this raise?

> class ModelTest(TestCase): 
>+    def test_domain_simple(self):

I might rename that "test_simple_domain".  Maybe not though.

This is a pretty good first patch.  There are some things to fix, and some stuff to figure out, but once those are fixed, it should be good to go.

Thanks,
Blake.

Comment 3

[David Ascher (:davida)]

Why do we have .hg directories in an svn tree anyway?   Anyway, svn and hg both have ways of specifying that diff should ignore specific filenames.  cf. svn:ignore

Comment 4

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #3)
> Why do we have .hg directories in an svn tree anyway?

The students were using hg with hg-svn to share patches amongst themselves while they were all getting the project up and running on their systems.

Later,
Blake.

Comment 5

[Ludovic Hirlimann [:Usul]]

confirming bug :-)

Comment 6

[Jay Schmidek]

Attachment: revised patch

requires Louie's patch, Bug 519916. I should have included those testing framework files in my previous patch :/

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 405006 [details] [diff] [review]
revised patch

First off, when I run it, I get:
FAILED (errors=2, failures=2)

Now, I don't mind failing tests in this case, because I know you're going to fix them in an upcoming patch (right?), but tests that throw errors aren't so good.

(The erroring tests are test_add_letters_port and test_add_missing_name.  And they both seem to be throwing TemplateDoesNotExist: config/enter_config.html)

Also, it looks like you're defining 11 tests, but it's only finding 4.  Maybe fixing the errors will let the others run.  Or maybe there's another problem there that's stopping them from running.  I think you should look into that a little.

>diff -r 8c39563c3e9d tests/test_add.py
>+from django.test import TestCase
>+from ispdb.config import views
>+from django.core.urlresolvers import reverse
>+from ispdb.config import models
>+from nose.tools import *
>+import datetime

We should probably order these somehow.  I like stealing processes from the Basie project, so perhaps we could follow their style guide, located at http://code.basieproject.org/trunk/STYLE

(In particular the line that says "Import one module per line.  Group them by stdlib, django, 3rd party, local."  Ask me if you don't know what category each of them should be put into.)

>+    def test_ask(self):
>+        self.client.post(reverse('ispdb_add'),{'asking_or_adding':'asking',
>+                                               'form-TOTAL_FORMS':'1',
>+                                               'form-INITIAL_FORMS':'1',
>+                                               'form-0-name':'ty.com'})
>+        domain = models.UnclaimedDomain.objects.get(name='ty.com')
>+        assert isinstance(domain,models.UnclaimedDomain)

I like to assert that there is no ty.com domain before we add it, just in case.

>+    def test_add_internationalization(self):
>+        name = u'Iñtërnâtiônàlizætiøn'

Nice.  :)

I wonder what happens if you add that as the domain name?

Later,
Blake.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #7)
> (From update of attachment 405006 [details] [diff] [review])
> First off, when I run it, I get:
> FAILED (errors=2, failures=2)
> 
> Also, it looks like you're defining 11 tests, but it's only finding 4.  Maybe
> fixing the errors will let the others run.  Or maybe there's another problem
> there that's stopping them from running.  I think you should look into that a
> little.

Wait, I take it back.  Above that line, it said:
Ran 12 tests in 4.845s

So that's all good.

Thanks,
Blake.

Comment 9

[Jay Schmidek]

(In reply to comment #7)
> (From update of attachment 405006 [details] [diff] [review])
> First off, when I run it, I get:
> FAILED (errors=2, failures=2)
> 
> Now, I don't mind failing tests in this case, because I know you're going to
> fix them in an upcoming patch (right?), but tests that throw errors aren't so
> good.
>
> (The erroring tests are test_add_letters_port and test_add_missing_name.  And
> they both seem to be throwing TemplateDoesNotExist: config/enter_config.html)

I haven't been able to reproduce this. Just the 2 tests fail for me, I don't get any errors. I tried under a fresh checkout in both windows and ubuntu.

Comment 10

[Jay Schmidek]

Attachment: new revised patch

a few small changes and one new test

Comment 11

[Jay Schmidek]

(In reply to comment #9)
> I haven't been able to reproduce this. Just the 2 tests fail for me, I don't
> get any errors. I tried under a fresh checkout in both windows and ubuntu.

I think I figured this out. It happens when I don't run manage.py directly from the ispdb folder, i.e. running it from ispdb's parent folder with
  python ispdb/manage.py test --settings=local_settings, when using Eric's patch (bug 518840)

This seems to stem from the local_settings.py script which sets the TEMPLATE_DIRS relative to the current path, which doesn't work unless you run it from within the ispdb folder.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 406959 [details] [diff] [review]
new revised patch

>+++ tests/test_add.py	(revision 0)

I'm still getting three failures:
.F..F.F......
======================================================================
FAIL: test_add_duplicate_domain (test_add.AddTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bwinton/Programming/thunderbird/ispdb/tests/test_add.py", line 105, in test_add_duplicate_domain
    assert_equal(res.status_code,200)
AssertionError: 302 != 200

======================================================================
FAIL: test_add_long_port (test_add.AddTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bwinton/Programming/thunderbird/ispdb/lib/python2.6/site-packages/nose/tools.py", line 80, in newfunc
    func(*arg, **kw)
  File "/Users/bwinton/Programming/thunderbird/ispdb/tests/test_add.py", line 161, in test_add_long_port
    assert_equal(res.status_code,200)
AssertionError: 302 != 200

======================================================================
FAIL: test_add_negative_port (test_add.AddTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bwinton/Programming/thunderbird/ispdb/lib/python2.6/site-packages/nose/tools.py", line 80, in newfunc
    func(*arg, **kw)
  File "/Users/bwinton/Programming/thunderbird/ispdb/tests/test_add.py", line 188, in test_add_negative_port
    assert_equal(res.status_code,200)
AssertionError: 302 != 200

----------------------------------------------------------------------
Ran 13 tests in 3.379s

But I could believe that they're bugs in the underlying code, and that your tests are correct.
(Would you mind looking into the code to make sure that it actually is bugs in the code causing the test failures, and not bugs in the tests?)

And don't forget to get the second review before marking it checkin-needed.  :)

Thanks,
Blake.

Comment 13

[Jay Schmidek]

(In reply to comment #12)
> But I could believe that they're bugs in the underlying code, and that your
> tests are correct.
> (Would you mind looking into the code to make sure that it actually is bugs in
> the code causing the test failures, and not bugs in the tests?)

Should have left a comment about that, I get 3 failures too. They are bugs in the code which are being addressed in bug:522250.

Comment 14

[Philippe M. Chiasson (:gozer)]

Comment on attachment 406959 [details] [diff] [review]
new revised patch

Looks good to me. Even better us that these highlight the bugs in bug 522250.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Checked in as:
http://viewvc.svn.mozilla.org/vc?view=revision&revision=54676

Congratulations, Jay!