521173 - Can't run any unit tests

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect)

Description

[David Ascher (:davida)]

+++ This bug was initially created as a clone of Bug #519916 +++

A particularly simple but interesting test would be a round-trip test which
imported from the static files being served from e.g.
http://live.mozillamessaging.com/autoconfig/aim.com through import, through
export, and comparing the outputs to the original input.

Comment 1

[Louie]

Currently we do not have an import mechanism that we can directly call (that I know of). The only time I did an import was during the installation, with python manage.py shell < import convert. Do you think it is worthwhile to wrap this up into a function so that we can preform this test?

Comment 2

[Louie]

Have a fix, but waiting for a review + checkin of Bug# 522253 because it is a dependency.

Comment 3

[Louie]

Attachment: Patch using aol config

AOL is a good XML without any of the extra fields that we are missing. A more rigorous test (using a set of different configs like googlemail) would fail.

Comment 4

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

Comment on attachment 417048 [details] [diff] [review]
Patch using aol config

So, this is a hard patch for me to review, because it conflicts a lot with my patch for bug 530972, which I need to have applied to run the unit tests, but we don't know which patch is going to make it in first, so I'm going to do my best to make general comments.

>Index: tests/test_xml.py
>@@ -54,3 +54,22 @@
>+    def roundtrip_test(self):

The rest of the tests have names like "test_foo", and "test_bar", so this should probably be renamed "test_roundtrip".

>+        fname = 'tests/aol.com'
>+        try:
>+            fsock = open(fname,'r');
>+            imported_xml = fsock.read()
>+            fsock.close()
>+        except IOError:
>+                assert False

I think we might be better off just letting the error get thrown, instead of catching it.  Then the test will report as "Error", instead of "Failed", which seems reasonable.

>+        utils.import_config(fname)
>+        domain = models.Domain.objects.get(name="aol.com")
>+        exported_xml = domain.config.as_xml();
>+        imported_xml = re.sub('[\s]+','',imported_xml)
>+        imported_xml = re.sub('\'','\"',imported_xml)

So, you control the imported xml, so you could change the 's to "s beforehand, and get rid of this line.
In fact, there are no 's in the imported xml, so you can just remove this line without doing anything.  :)

>+        exported_xml = re.sub('[\s]+','',exported_xml)
>+        exported_xml = re.sub('\'','\"',exported_xml)

And does the exported_xml have different indentation than the imported_xml, or did you just do this for safety?  Because I would love it if we could check that the indentation was the same.  :)

I'ld also love it if you could run the test against all the files in the directory, and let us know what the differences are, if any.  (Not as a unit test, just for curiosity's sake.
Perhaps as a flag to convert.py?  Feel free to say "no", and I'll log it as a separate bug.)

>+        #nose.tools.set_trace()

We should remove this comment.

>Index: tests/aol.com
>Index: config/utils.py
>@@ -0,0 +1,54 @@
>+import os, sys
>+from StringIO import StringIO
>+
>+from ispdb.config.models import Domain, Config
>+try:
>+  import xml.etree.ElementTree as ET
>+except ImportError:
>+  import elementtree.ElementTree as ET

Since we already need to have lxml installed, we should just use:
import lxml.etree as ET

>+def import_config(xml_config):
>+    """ Used to import xml_config from a file, file object or string """ 
>+    if type(xml_config) == file:
>+            fname = xml_config;

I think you've got too many spaces there.

>+    elif os.path.isfile(xml_config):
>+            fname = os.path.join(os.getcwd(),xml_config)
>+            print "importing %s" % fname
>+    else:
>+            fname = StringIO(xml_config)

I think we might want to make this simpler, and just insist that the caller pass in a file-like object (which could be a file or a StringIO).  Yes, that makes them open the file, but it makes our API much simpler.

>+    print "PROCESSING", fname

It also means that we can't print this, but that's a good thing, because utility functions shouldn't really be printing stuff.  :)

>+    et = ET.parse(fname)
>+    root = et.getroot()
>+    #print root

I think we can remove this comment.

>+    incoming = root.find('.//incomingServer')
>+    outgoing = root.find('.//outgoingServer')
>+    id = root.find(".//emailProvider").attrib['id']
>+    # if we have one for this id, skip it
>+    if Config.objects.filter(email_provider_id=id):
>+            print "Already have config for %s" % id
>+            return 0

I don't think printing something is the right thing to do here.
Perhaps we should have an argument which indicates whether we should throw an exception or not.

It's also really strange to return 0 sometimes, and other times not return anything.  So, we should figure out whether trying to add a duplicate id is an exceptional case or not.  If it is, then we should throw an exception; if it isn't we should figure out what we want to return in the successful and unsuccessful cases.  (Perhaps we could return the newly-created Config or None?)

>+    c = Config(id=None,
>+             email_provider_id = id,
>+             display_name = root.find('.//displayName').text,

Please use "s, if you're moving the code to a new location.
And the indentation is odd here.

>+    domains = root.findall('.//domain')
>+    ds = [Domain(id=None, name=domain.text, config=c) for domain in domains]
>+    for d in ds:
>+        d.save()

We don't complain about overwriting duplicate domains?
Should we?

>Index: convert.py
>@@ -6,45 +7,8 @@
> except ImportError:
>   import elementtree.ElementTree as ET
> 
>-def main():
>-  datadir = os.environ.get("AUTOCONFIG_DATA", "../autoconfig_data")
>-  for fname in os.listdir(datadir):
>-    if fname.startswith('.') or fname == "README": continue
>-    print "PROCESSING", fname
[...]
>-
>-if __name__ == "__main__":
>-  main()
>+datadir = os.environ.get("AUTOCONFIG_DATA", "../autoconfig_data")
>+for fname in os.listdir(datadir):
>+  if fname.startswith('.') or fname == "README": continue
>+  utils.import_config(os.path.join(datadir,fname))

I'm pretty sure we still only want to do this if __name__ == "__main__".

Thanks,
Blake.