no unit tests for adding domains to ispdb

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: jschmidek, Assigned: jschmidek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

10 years ago
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
Assignee

Comment 1

10 years ago
Attachment #403115 - Flags: review?(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.
Attachment #403115 - Flags: review?(bwinton) → review-
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
(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.
confirming bug :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 6

10 years ago
Posted patch revised patch (obsolete) — Splinter Review
requires Louie's patch, Bug 519916. I should have included those testing framework files in my previous patch :/
Attachment #403115 - Attachment is obsolete: true
Attachment #405006 - Flags: review?(bwinton)
Attachment #405006 - Flags: review?(bwinton) → review-
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.
(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.
Assignee: nobody → jschmidek
Status: NEW → ASSIGNED
Assignee

Comment 9

10 years ago
(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.
Assignee

Comment 10

10 years ago
a few small changes and one new test
Attachment #405006 - Attachment is obsolete: true
Attachment #406959 - Flags: review?(bwinton)
Assignee

Comment 11

10 years ago
(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.
Priority: -- → P1
Attachment #406959 - Flags: review?(bwinton) → review+
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.
Assignee

Comment 13

10 years ago
(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.
Assignee

Updated

10 years ago
Attachment #406959 - Flags: review?(david.ascher)
Assignee

Updated

10 years ago
Attachment #406959 - Flags: review?(david.ascher) → review?(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.
Attachment #406959 - Flags: review?(gozer) → review+
Assignee

Updated

10 years ago
Keywords: checkin-needed
Checked in as:
http://viewvc.svn.mozilla.org/vc?view=revision&revision=54676

Congratulations, Jay!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 665884
No longer blocks: 665884
Component: ispdb → ISPDB Server
Product: Mozilla Messaging → Webtools
You need to log in before you can comment on or make changes to this bug.