Merge lp:~gmb/maas-test/add-reporting into lp:maas-test

[Graham Binns]

Add reporting-via Launchpad.

This branch adds reporting-by-Launchpad to maas-test.

To make this work, I've:

 - Added a way of buffering the logs and test results so that we can save them or email them.
 - Added a report.py module, which builds the blob and uploads it to Launchpad (lots of this was cargo-culted from elsewhere).
 - Added tests for the above.

As a drive-by, I've also fixed the memory-limit bug that I discovered earlier this week (on i386 systems, qemu errors if --memory is > 2047).

One of the functions in report.py isn't tested, annoyingly, and I'll work on fixing that, but I'd rather get this code landed _now_.

[Raphaël Badin (rvb)]

[0]

40 + # We specifically use 2047 for the memory size here because
41 + # using 2048 causes qemu to raise an error on i386 hosts.
42 + "--memory", "2047",

This will conflict with what's in trunk now. Please merge trunk.

[1]

The module-level docstrings of the new modules need to be changed.

[2]

maastest/main.py

80 + from maastest import report
81 + encoded_results = (
82 + output_stream.buffer.getvalue().encode('utf-8'))
83 + blob = report.create_launchpad_blob(encoded_results)
84 + blob_token = report.upload_blob(blob)
[...]
100 + "Visit https://bugs.launchpad.net/maas-test/+filebug/%s "
101 + "to file a bug and complete the maas-test reporting process."
102 + % blob_token)

Wouldn't it be better if all of this was in a separate helper method so that it can be easily tested?

[3]

27 +__all__ = [
128 + "upload_blob",
129 + ]

'create_launchpad_blob' is missing from the list.

[4]

This while thing is a pretty serious change to what maas-test does. A good one I mean, but a serious one and I don't see any changes to the documentation. Maybe you're planning to do it in a follow-up branch but it's pretty important that this behavior is documented.

[5]

Questions about testing: what the code does is pretty tricky as it interacts with an external system (Launchpad). Can I ask what kind of testing you've done already? Could we have sort of a QA plan for this? In particular, I think it's pretty important to make sure that everything works fine when maas-test can't access Launchpad; I don't think there is provision for that in the code: what happens if upload_blob.opener.open(request) errors for instance?

[6]

This code doesn't use the optionally-provided proxy option. This does not seem right to me but I'd like to hear your thoughts about this.

[7]

Maybe something we can do in a follow-up branch but, although I think it's sane to do the reporting by default, I'm also convinced it would be pretty useful to have a "--no-report" option.

[Raphaël Badin (rvb)]

[8]

448 +class BufferingOutputStream:
449 +

This, I think, deserves a docstring.

[9]

8 +from maastest import utils
9 +import sys
10 import unittest
11
12 -from maastest import utils
13 -
14 -

Why moving "from maastest import utils"? I suggest using MAAS' ./utilities/format-imports script on this file to get the imports sorted (and nicely regrouped based on where they come from).

[10]

1 === modified file 'maastest/console.py'
[...]
23 + if output_stream is None:
24 + output_stream = sys.stderr

Is that used?

[11]

92 + log_file_path = os.path.join(
93 + BaseDirectory.save_config_path('maas-test'), 'maas-test.log')
94 + with open(log_file_path, 'w') as log_file:
95 + log_file.write(encoded_results)

Looks like multiple runs (even if testing different nodes) will all write to the same output log. I think we should include something in the name (a timestamp? the BMC's MAC address if provided? Both?) of the output file. Also, in this case, it's probably worth putting them all in ~/.maas-test/logs or something. Finally, but up to you really, I'd move that logic into a dedicated helper.

[12]

159 +def create_launchpad_blob(test_results):
[...]
168 + results_payload = make_file_payload(
169 + 'maas-test.log', test_results.encode('ascii'))

178 +def build_upload_mime_multipart(blob):
[...]
187 + form_blob_field.add_header(
188 + 'Content-Disposition', 'form-data; name="field.blob"; filename="x"')
189 + form_blob_field.set_payload(blob.as_string().encode('ascii'))

81 + encoded_results = (
82 + output_stream.buffer.getvalue().encode('utf-8'))

Why are we encoding stuff in ascii and later on in "UTF-8"? This might be my ignorance of what's going on inside Launchpad speaking but I'd like to better understand what's happening here… (It's probably worth a comment.)

[13]

165 + launchpad_form_data['subject'] = "MAAS Test results for: "

Again, I'm confused by the "truncated" subject because my ignorance of what's happening on the Launchpad side… but why is the 'subject' like this? (It's probably worth a comment.)

[Raphaël Badin (rvb)]

[14]

Another remark (which, again, could probably be addressed in a follow-up branch): having an option to force maas-test to write the report into a file instead of sending it to LP is probably a good idea (in addition to the "--no-report" option, see [7]).

[Graham Binns (gmb)]

> [0]
>
> 40 + # We specifically use 2047 for the memory size here
> because
> 41 + # using 2048 causes qemu to raise an error on i386 hosts.
> 42 + "--memory", "2047",
>
> This will conflict with what's in trunk now. Please merge trunk.

Done.

>
> [1]
>
> The module-level docstrings of the new modules need to be changed.

Done.

> [2]
>
> maastest/main.py
>
> 80 + from maastest import report
> 81 + encoded_results = (
> 82 + output_stream.buffer.getvalue().encode('utf-8'))
> 83 + blob = report.create_launchpad_blob(encoded_results)
> 84 + blob_token = report.upload_blob(blob)
> [...]
> 100 + "Visit https://bugs.launchpad.net/maas-test/+filebug/%s "
> 101 + "to file a bug and complete the maas-test reporting
> process."
> 102 + % blob_token)
>
> Wouldn't it be better if all of this was in a separate helper method so that
> it can be easily tested?

Yes, Done.

> [3]
>
> 27 +__all__ = [
> 128 + "upload_blob",
> 129 + ]
>
> 'create_launchpad_blob' is missing from the list.

Fixed.

> [4]
>
> This while thing is a pretty serious change to what maas-test does. A good
> one I mean, but a serious one and I don't see any changes to the
> documentation. Maybe you're planning to do it in a follow-up branch but it's
> pretty important that this behavior is documented.

I was planning to do it in a follow-up branch, for the sake of keeping this one as bloated as it is right now.

> [5]
>
> Questions about testing: what the code does is pretty tricky as it interacts
> with an external system (Launchpad). Can I ask what kind of testing you've
> done already? Could we have sort of a QA plan for this? In particular, I
> think it's pretty important to make sure that everything works fine when maas-
> test can't access Launchpad; I don't think there is provision for that in the
> code: what happens if upload_blob.opener.open(request) errors for instance?

I've done plenty of testing of how it interacts with Launchpad, but only when Launchpad was available (foolishly enough). It now handles correctly not being able to connect to Launchpad.

I'll add more in a subsequent branch.

> [6]
>
> This code doesn't use the optionally-provided proxy option. This does not
> seem right to me but I'd like to hear your thoughts about this.

No, you're right it, it doesn't. That's fixed now by setting the http(s)_proxy in os.environ (though I now feel the proxy setup is done in slightly the wrong place, but that's a concern for another time).

> [7]
>
> Maybe something we can do in a follow-up branch but, although I think it's
> sane to do the reporting by default, I'm also convinced it would be pretty
> useful to have a "--no-report" option.

There'll be a follow up branch for that.

[Graham Binns (gmb)]

> [8]
>
> 448 +class BufferingOutputStream:
> 449 +
>
> This, I think, deserves a docstring.
>
> [9]
>
> 8 +from maastest import utils
> 9 +import sys
> 10 import unittest
> 11
> 12 -from maastest import utils
> 13 -
> 14 -
>
> Why moving "from maastest import utils"? I suggest using MAAS' ./utilities
> /format-imports script on this file to get the imports sorted (and nicely
> regrouped based on where they come from).

Good point. I'll do that in another branch; it reformats imports in most of the .py files under /maastest

> [10]
>
> 1 === modified file 'maastest/console.py'
> [...]
> 23 + if output_stream is None:
> 24 + output_stream = sys.stderr
>
> Is that used?

Nope. Removed.

> [11]
>
> 92 + log_file_path = os.path.join(
> 93 + BaseDirectory.save_config_path('maas-test'), 'maas-
> test.log')
> 94 + with open(log_file_path, 'w') as log_file:
> 95 + log_file.write(encoded_results)
>
> Looks like multiple runs (even if testing different nodes) will all write to
> the same output log. I think we should include something in the name (a
> timestamp? the BMC's MAC address if provided? Both?) of the output file.
> Also, in this case, it's probably worth putting them all in ~/.maas-test/logs
> or something. Finally, but up to you really, I'd move that logic into a
> dedicated helper.

Done (timestamp; I'll add the BMC MAC in another branch). Also moved to a helper.

> [12]
>
> 159 +def create_launchpad_blob(test_results):
> [...]
> 168 + results_payload = make_file_payload(
> 169 + 'maas-test.log', test_results.encode('ascii'))
>
> 178 +def build_upload_mime_multipart(blob):
> [...]
> 187 + form_blob_field.add_header(
> 188 + 'Content-Disposition', 'form-data; name="field.blob";
> filename="x"')
> 189 + form_blob_field.set_payload(blob.as_string().encode('ascii'))
>
> 81 + encoded_results = (
> 82 + output_stream.buffer.getvalue().encode('utf-8'))
>
> Why are we encoding stuff in ascii and later on in "UTF-8"? This might be my
> ignorance of what's going on inside Launchpad speaking but I'd like to better
> understand what's happening here… (It's probably worth a comment.)

No, that's cargo-culted code. I've no idea why it happens but I've removed it and it seems still to work.

> [13]
>
> 165 + launchpad_form_data['subject'] = "MAAS Test results for: "
>
> Again, I'm confused by the "truncated" subject because my ignorance of what's
> happening on the Launchpad side… but why is the 'subject' like this? (It's
> probably worth a comment.)

This is so that someone can fill in the details of what the test was run against, but I guess we could just leave it blank and trust them to be smart enough to use a sensible subject line.

[Graham Binns (gmb)]

> [14]
>
> Another remark (which, again, could probably be addressed in a follow-up
> branch): having an option to force maas-test to write the report into a file
> instead of sending it to LP is probably a good idea (in addition to the "--no-
> report" option, see [7]).

Yes, I'll do that in my follow-up branch.

[Gavin Panella (allenap)]

Looks good. This is excellent stuff to get reporting working with
Launchpad, and it's apparent that a lot of work has gone into this.
Things like the Launchpad blob prep and upload code could have been
reviewed and landed independently, but things don't always work out as
clear cut as we'd like. I have more comments to pile onto the ones that
Raffers has already made, but I'm not going to binary fissionate
post-midnight-snacking mogwai if you disagree with them.

[1]

@@ -70,6 +71,8 @@
             # just use that proxy for everything.
             proxy_url = cls.args.http_proxy
             logging.info("Using external proxy %s." % proxy_url)
+            os.environ['http_proxy'] = cls.args.http_proxy
+            os.environ['https_proxy'] = cls.args.https_proxy
         elif cls.args.disable_cache:
             # The user has passed --disable-cache, so don't start polipo
             # and don't try to use an external proxy.
@@ -81,6 +84,8 @@
             proxy = LocalProxyFixture()
             cls.fixtures.useFixture(proxy)
             proxy_url = proxy.get_url()
+            os.environ['http_proxy'] = proxy_url
+            os.environ['https_proxy'] = proxy_url

Can you use fixtures.EnvironmentVariable here?

    from fixtures import EnvironmentVariable
    ...

    @classmethod
    def useProxy(cls, url):
        cls.fixtures.useFixture(EnvironmentVariable("http_proxy", url))
        cls.fixtures.useFixture(EnvironmentVariable("https_proxy", url))

    @classmethod
    def setUpClass(cls):
        super(TestMAAS, cls).setUpClass()

        # TODO: series and architecture should be script parameters.
        architecture = utils.determine_vm_architecture()

        if cls.args.http_proxy is not None:
            # The user has passed an external proxy; don't start polipo,
            # just use that proxy for everything.
            proxy_url = cls.args.http_proxy
            cls.useProxy(proxy_url)
            logging.info("Using external proxy %s." % proxy_url)
        elif cls.args.disable_cache:
            # The user has passed --disable-cache, so don't start polipo
            # and don't try to use an external proxy.
            proxy_url = ''
            cls.useProxy(None)
            logging.info("Caching disabled.")
        else:
            # The default case: start polipo and use that as our caching
            # proxy.
            proxy = LocalProxyFixture()
            cls.fixtures.useFixture(proxy)
            proxy_url = proxy.get_url()
            cls.useProxy(proxy_url)

[2]

        cls.machine = KVMFixture(
            series=cls.args.maas_series, architecture=architecture,
            proxy_url=proxy_url, direct_interface=cls.args.interface,
            archives=cls.args.archive)

You've not changed this, but it seem odd now to set the proxy in the
environment *and* pass it into KVMFixture. Also, KVMFixture has a few
commands where it sets http_proxy in the environment, for example:

        return [
            'sudo', 'http_proxy=%s' % self.proxy_url,
            'DEBIAN_FRONTEND=noninteractive',
            'apt-get', 'install', '-y'
            ] + list(packages)

Now that we're setting these in the environme...

review: Approve

[Graham Binns (gmb)]

> Looks good. This is excellent stuff to get reporting working with
> Launchpad, and it's apparent that a lot of work has gone into this.
> Things like the Launchpad blob prep and upload code could have been
> reviewed and landed independently, but things don't always work out as
> clear cut as we'd like. I have more comments to pile onto the ones that
> Raffers has already made, but I'm not going to binary fissionate
> post-midnight-snacking mogwai if you disagree with them.

Best review comment ever.

> [1]
>
> @@ -70,6 +71,8 @@
> # just use that proxy for everything.
> proxy_url = cls.args.http_proxy
> logging.info("Using external proxy %s." % proxy_url)
> + os.environ['http_proxy'] = cls.args.http_proxy
> + os.environ['https_proxy'] = cls.args.https_proxy
> elif cls.args.disable_cache:
> # The user has passed --disable-cache, so don't start polipo
> # and don't try to use an external proxy.
> @@ -81,6 +84,8 @@
> proxy = LocalProxyFixture()
> cls.fixtures.useFixture(proxy)
> proxy_url = proxy.get_url()
> + os.environ['http_proxy'] = proxy_url
> + os.environ['https_proxy'] = proxy_url
>
> Can you use fixtures.EnvironmentVariable here?

I don't think so, because (rather filthily; I don't like it and it needs
refactoring in some way) doing the os.environ dance means that when we
do the reporting, which happens outside of the TestCase, the proxy still
gets used automatically by urllib2. Of course, we could handle it
manually, but that doesn't feel any nicer.

> [2]
>
> cls.machine = KVMFixture(
> series=cls.args.maas_series, architecture=architecture,
> proxy_url=proxy_url, direct_interface=cls.args.interface,
> archives=cls.args.archive)
>
> You've not changed this, but it seem odd now to set the proxy in the
> environment *and* pass it into KVMFixture.

That's true, and I'm going to address that in a follow-up branch.

> Also, KVMFixture has a few commands where it sets http_proxy in the
> environment, for example:
>
> return [
> 'sudo', 'http_proxy=%s' % self.proxy_url,
> 'DEBIAN_FRONTEND=noninteractive',
> 'apt-get', 'install', '-y'
> ] + list(packages)
>
> Now that we're setting these in the environment, it ought to work to do:
>
> return [
> 'sudo', '-E',
> ...
>
> to pass through the caller's environment.
>
> Don't do this now though, just wondering what your thoughts are.

That would work for some of the commands but not all of them. For
example, as Raffers and I discovered earlier this week,
uvtool-simplestreams, to which we shell out, uses the requests module,
which doesn't handle having an https_proxy at all well (in point of
fact, it pukes on your shoes should you specify one). So we could use -E
in some places but not others. I'm fine with that if it's just for the
sync command, where we're downloading the remote images.VGV

> [3]
>
> + results_payload = make_file_payload(
> + 'maas-test.log', test_results.encode('ascii'))
>...

=== modified file 'maastest/cases.py'
--- maastest/cases.py	2013-12-12 16:23:47 +0000
+++ maastest/cases.py	2013-12-17 07:45:37 +0000
@@ -17,6 +17,7 @@
 import httplib
 import json
 import logging
+import os
 import sys
 
 from fixtures import Fixture
@@ -70,6 +71,8 @@
             # just use that proxy for everything.
             proxy_url = cls.args.http_proxy
             logging.info("Using external proxy %s." % proxy_url)
+            os.environ['http_proxy'] = cls.args.http_proxy
+            os.environ['https_proxy'] = cls.args.https_proxy
         elif cls.args.disable_cache:
             # The user has passed --disable-cache, so don't start polipo
             # and don't try to use an external proxy.
@@ -81,6 +84,8 @@
             proxy = LocalProxyFixture()
             cls.fixtures.useFixture(proxy)
             proxy_url = proxy.get_url()
+            os.environ['http_proxy'] = proxy_url
+            os.environ['https_proxy'] = proxy_url
 
         cls.machine = KVMFixture(
             series=cls.args.maas_series, architecture=architecture,

=== modified file 'maastest/console.py'
--- maastest/console.py	2013-12-05 14:02:09 +0000
+++ maastest/console.py	2013-12-17 07:45:37 +0000
@@ -14,12 +14,11 @@
     "run_console",
     ]
 
+from maastest import utils
 import unittest
 
-from maastest import utils
-
-
-def run_console(testcase):
+
+def run_console(testcase, output_stream):
     """Run the tests found on `testcase`, reporting to the console.
 
     :type testcase: `unittest.TestCase`
@@ -28,5 +27,5 @@
     loader = utils.CasesLoader()
     suite = loader.loadTestsFromTestCase(testcase)
     runner = unittest.TextTestRunner(
-        verbosity=2, descriptions=False, failfast=True)
+        verbosity=2, descriptions=False, failfast=True, stream=output_stream)
     return runner.run(suite)

=== modified file 'maastest/main.py'
--- maastest/main.py	2013-12-06 11:59:53 +0000
+++ maastest/main.py	2013-12-17 07:45:37 +0000
@@ -37,9 +37,14 @@
     from maastest.parser import prepare_parser
     args = prepare_parser().parse_args(argv)
 
+    from maastest import utils
     import logging
+    # We tie everything onto one output stream so that we can capture
+    # things for reporting.
+    output_stream = utils.BufferingOutputStream(sys.stdout)
     logging.basicConfig(
-        level=logging.DEBUG, format='%(asctime)s %(levelname)s %(message)s')
+        level=logging.DEBUG, format='%(asctime)s %(levelname)s %(message)s',
+        stream=output_stream)
 
     import os
     if os.geteuid() != 0:
@@ -86,15 +91,18 @@
     # up when a KeyboardInterrupt exception is raised.
     # This does miss out on test-level and module-level tearDowns.
     try:
-        result = run_console(ConfiguredTestMAAS)
+        result = run_console(ConfiguredTestMAAS, output_stream)
     except KeyboardInterrupt:
         ConfiguredTestMAAS.tearDownClass()
         raise
+    from maastest import report
+    encoded_results = (
+        output_stream.buffer.getvalue().encode('utf-8'))
+    report.report_test_results(encoded_results)
+    if result.wasSuccessful():
+        return RETURN_CODES.SUCCESS
     else:
-        if result.wasSuccessful():
-            return RETURN_CODES.SUCCESS
-        else:
-            return RETURN_CODES.TEST_FAILED
+        return RETURN_CODES.TEST_FAILED
 
 
 if __name__ == '__main__':

=== added file 'maastest/report.py'
--- maastest/report.py	1970-01-01 00:00:00 +0000
+++ maastest/report.py	2013-12-17 07:45:37 +0000
@@ -0,0 +1,150 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Reporting functions for MAAS test."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    "create_launchpad_blob",
+    "upload_blob",
+    ]
+
+
+from apiclient import multipart
+from io import BytesIO
+from urllib2 import (
+    HTTPSHandler,
+    Request,
+    build_opener,
+    )
+import email.mime
+import logging
+from datetime import datetime
+import os
+from xdg import BaseDirectory
+
+
+# This is largely cargo-culted from apiclient.multipart because
+# Launchpad only pays attention to the first Content-Disposition header
+# on an attachment, and if we want the content to actually be attached
+# to the bug rather than lost in the ether we have to make it
+# Content-Disposition: attachment. By default,
+# apiclient.multipart.make_file_payload() gives the payload a
+# Content-Disposition of "form-data".
+def make_file_payload(name, content):
+    payload = email.mime.application.MIMEApplication(content)
+    payload.add_header(
+        "Content-Disposition", "attachment", name=name, filename=name)
+    names = name, getattr(content, "name", None)
+    payload.set_type(multipart.get_content_type(*names))
+    return payload
+
+
+def create_launchpad_blob(test_results):
+    """Create an RFC822-formatted blob of data to upload to Launchpad.
+    """
+    launchpad_form_data = multipart.build_multipart_message([])
+    launchpad_form_data['tags'] = 'maas-test-results'
+    launchpad_form_data['private'] = 'yes'
+    launchpad_form_data['subscribers'] = "private-canonical-maas"
+
+    results_payload = make_file_payload(
+        'maas-test.log', test_results.encode('ascii'))
+    launchpad_form_data.attach(results_payload)
+
+    return launchpad_form_data
+
+
+# And this is largely cargo-culted from apport.crashdb_impl.launchpad,
+# because it does all the form-submission dance that we need to do to
+# make this actually work.
+def build_upload_mime_multipart(blob):
+    launchpad_form_data = email.mime.multipart.MIMEMultipart()
+
+    submit = email.mime.Text.MIMEText('1')
+    submit.add_header(
+        'Content-Disposition', 'form-data; name="FORM_SUBMIT"')
+    launchpad_form_data.attach(submit)
+
+    form_blob_field = email.mime.Base.MIMEBase('application', 'octet-stream')
+    form_blob_field.add_header(
+        'Content-Disposition', 'form-data; name="field.blob"; filename="x"')
+    form_blob_field.set_payload(blob.as_string().encode('ascii'))
+    launchpad_form_data.attach(form_blob_field)
+
+    return launchpad_form_data
+
+
+def build_upload_request(url, launchpad_form_data):
+    data_flat = BytesIO()
+    generator = email.generator.Generator(data_flat, mangle_from_=False)
+    generator.flatten(launchpad_form_data)
+
+    # do the request; we need to explicitly set the content type here, as it
+    # defaults to x-www-form-urlencoded
+    request = Request(url, data_flat.getvalue())
+    request.add_header(
+        'Content-Type',
+        'multipart/form-data; boundary=' + launchpad_form_data.get_boundary())
+
+    return request
+
+
+def upload_blob(blob, hostname='launchpad.net'):
+    """Upload blob (file-like object) to Launchpad.
+
+    :param blob:
+    """
+    token = None
+    url = 'https://%s/+storeblob' % hostname
+    form = build_upload_mime_multipart(blob)
+    request = build_upload_request(url, form)
+    opener = build_opener(HTTPSHandler)
+    try:
+        result = opener.open(request)
+    except Exception as error:
+        logging.error("Unable to connect to Launchpad: %s" % error.message)
+        return None
+    else:
+        token = result.info().get('X-Launchpad-Blob-Token')
+        return token
+
+
+def write_test_results(log_file_name, results, log_dir=None):
+    if log_dir is None:
+        log_file_path = os.path.join(
+            BaseDirectory.save_config_path('maas-test'),
+            'logs', log_file_name)
+    else:
+        log_file_path = os.path.join(log_dir, log_file_name)
+    with open(log_file_path, 'w') as log_file:
+        log_file.write(results)
+    logging.info("Test log saved to %s." % log_file_path)
+
+
+def report_test_results(results, now=None):
+    blob = create_launchpad_blob(results)
+    blob_token = upload_blob(blob)
+
+    if blob_token is None:
+        # For some reason the upload failed, so we save the contents of
+        # the buffer to a file and tell the user where it is.
+        logging.info("Unable to upload test results to Launchpad.")
+        if now is None:
+            now = datetime.now()
+
+        log_file_name = (
+            "maas-test.%s.log" % now.strftime("%Y-%m-%d_%H:%M:%S"))
+        write_test_results(log_file_name, results)
+    else:
+        logging.info("Test results uploaded to Launchpad.")
+        logging.info(
+            "Visit https://bugs.launchpad.net/maas-test/+filebug/%s "
+            "to file a bug and complete the maas-test reporting process."
+            % blob_token)

=== added file 'maastest/tests/test_report.py'
--- maastest/tests/test_report.py	1970-01-01 00:00:00 +0000
+++ maastest/tests/test_report.py	2013-12-17 07:45:37 +0000
@@ -0,0 +1,263 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `maastest.report`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+
+from fixtures import TempDir
+import base64
+import email
+import io
+import os.path
+from maastest import report
+import testtools
+import urllib2
+import logging
+import mock
+from datetime import datetime
+
+
+class TestMakeFilePayload(testtools.TestCase):
+
+    def test_returns_MIMEApplication(self):
+        payload = report.make_file_payload(
+            self.getUniqueString(), self.getUniqueString())
+        self.assertIsInstance(
+            payload, email.mime.application.MIMEApplication)
+
+    def test_sets_content_disposition_header(self):
+        filename = self.getUniqueString()
+        payload = report.make_file_payload(
+            filename, self.getUniqueString())
+        expected_disposition = (
+            'attachment; name="%(name)s"; filename="%(name)s"' %
+            {'name': filename})
+        self.assertEqual(
+            expected_disposition, payload['Content-Disposition'])
+
+    def test_sets_payload(self):
+        content = self.getUniqueString()
+        filename = self.getUniqueString()
+        payload = report.make_file_payload(filename, content)
+        encoded_content = base64.b64encode(content)
+        self.assertEqual(
+            encoded_content,
+            payload.get_payload().replace("\n", ""))
+
+
+class TestCreateLaunchpadBlob(testtools.TestCase):
+
+    def test_returns_MIMEMultipart(self):
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertIsInstance(blob, email.mime.multipart.MIMEMultipart)
+
+    def test_sets_tags_header(self):
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertEqual("maas-test-results", blob['tags'])
+
+    def test_sets_private_header(self):
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertEqual("yes", blob['private'])
+
+    def test_sets_subscribers_header(self):
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertEqual("private-canonical-maas", blob['subscribers'])
+
+    def test_attaches_results_as_payload(self):
+        results = self.getUniqueString()
+        blob = report.create_launchpad_blob(results)
+        expected_payload = report.make_file_payload("maas-test.log", results)
+        actual_payload = blob.get_payload()[0]
+        self.assertEqual(
+            expected_payload['Content-Disposition'],
+            actual_payload['Content-Disposition'])
+        self.assertEqual(
+            expected_payload.get_payload(), actual_payload.get_payload())
+
+
+class TestBuildUploadMIMEMultipart(testtools.TestCase):
+
+    def setUp(self):
+        super(TestBuildUploadMIMEMultipart, self).setUp()
+        self.launchpad_blob = report.create_launchpad_blob(
+            self.getUniqueString())
+
+    def test_returns_MIMEMultipart(self):
+        upload_multipart = report.build_upload_mime_multipart(
+            self.launchpad_blob)
+        self.assertIsInstance(
+            upload_multipart,
+            email.mime.multipart.MIMEMultipart)
+
+    def test_adds_submit_payload(self):
+        upload_multipart = report.build_upload_mime_multipart(
+            self.launchpad_blob)
+        submit_payload = upload_multipart.get_payload()[0]
+        self.assertIsInstance(submit_payload, email.mime.Text.MIMEText)
+        self.assertEqual(
+            submit_payload['Content-Disposition'],
+            'form-data; name="FORM_SUBMIT"')
+        self.assertEqual('1', submit_payload.get_payload())
+
+    def test_adds_blob_as_payload(self):
+        upload_multipart = report.build_upload_mime_multipart(
+            self.launchpad_blob)
+        blob_payload = upload_multipart.get_payload()[1]
+        self.assertEqual(
+            blob_payload['Content-Disposition'],
+            'form-data; name="field.blob"; filename="x"')
+        self.assertEqual(
+            self.launchpad_blob.as_string().encode('ascii'),
+            blob_payload.get_payload())
+
+
+class TestBuildUploadRequest(testtools.TestCase):
+
+    def setUp(self):
+        super(TestBuildUploadRequest, self).setUp()
+        self.launchpad_blob = report.create_launchpad_blob(
+            self.getUniqueString())
+        self.launchpad_form_data = report.build_upload_mime_multipart(
+            self.launchpad_blob)
+
+    def test_returns_http_request(self):
+        request = report.build_upload_request(
+            "http://example.com", self.launchpad_form_data)
+        self.assertIsInstance(request, urllib2.Request)
+
+    def test_sets_request_content_type_header(self):
+        request = report.build_upload_request(
+            "http://example.com", self.launchpad_form_data)
+        self.assertRegexpMatches(
+            request.get_header("Content-type"),
+            'multipart/form-data; boundary=.*')
+
+    def test_sets_request_data(self):
+        flat_buffer = io.BytesIO()
+        generator = email.generator.Generator(flat_buffer, mangle_from_=False)
+        generator.flatten(self.launchpad_form_data)
+        flattened_data = flat_buffer.getvalue()
+
+        request = report.build_upload_request(
+            "http://example.com", self.launchpad_form_data)
+        self.assertEqual(flattened_data, request.data)
+
+
+class TestUploadBlob(testtools.TestCase):
+
+    def setUp(self):
+        super(TestUploadBlob, self).setUp()
+        self.mock_opener = mock.MagicMock()
+        self.mock_result = mock.MagicMock()
+        self.mock_opener.open = mock.MagicMock(return_value=self.mock_result)
+        self.patch(
+            report, 'build_opener',
+            mock.MagicMock(return_value=self.mock_opener))
+
+    def test_builds_opener(self):
+        results = self.getUniqueString()
+        blob = report.create_launchpad_blob(results)
+        mock_build_opener = mock.MagicMock(return_value=self.mock_opener)
+        self.patch(report, 'build_opener', mock_build_opener)
+        report.upload_blob(blob)
+        mock_build_opener.assert_has_calls(
+            [mock.call(urllib2.HTTPSHandler)])
+
+    def test_opens_url(self):
+        results = self.getUniqueString()
+        blob = report.create_launchpad_blob(results)
+        mock_request = mock.MagicMock()
+        mock_build_upload_request = mock.MagicMock(
+            return_value=mock_request)
+        self.patch(report, 'build_upload_request', mock_build_upload_request)
+        report.upload_blob(blob)
+        self.mock_opener.open.assert_has_calls([mock.call(mock_request)])
+
+    def test_returns_token(self):
+        token = self.getUniqueString()
+        mock_info = mock.MagicMock(
+            return_value={'X-Launchpad-Blob-Token': token})
+        self.mock_result.info = mock_info
+
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertEqual(token, report.upload_blob(blob))
+
+    def test_returns_none_if_request(self):
+        self.mock_opener.open.return_value = None
+        self.mock_opener.open.side_effect = Exception(self.getUniqueString())
+        mock_error = mock.MagicMock()
+        self.patch(logging, 'error', mock_error)
+
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        expected_message = (
+            'Unable to connect to Launchpad: %s' %
+            self.mock_opener.open.side_effect.message)
+        token = report.upload_blob(blob)
+        mock_error.assert_has_calls([mock.call(expected_message)])
+        self.assertEqual(None, token)
+
+
+class TestReportTestResults(testtools.TestCase):
+
+    def test_uploads_blob(self):
+        token = self.getUniqueString()
+        blob = email.mime.multipart.MIMEMultipart()
+        mock_logger = mock.MagicMock()
+        mock_create_launchpad_blob = mock.MagicMock(return_value=blob)
+        mock_upload_blob = mock.MagicMock(return_value=token)
+
+        self.patch(logging, 'info', mock_logger)
+        self.patch(report, 'create_launchpad_blob', mock_create_launchpad_blob)
+        self.patch(report, 'upload_blob', mock_upload_blob)
+
+        report.report_test_results(self.getUniqueString())
+        mock_upload_blob.assert_has_calls([mock.call(blob)])
+        mock_logger.assert_has_calls([
+            mock.call("Test results uploaded to Launchpad."),
+            mock.call(
+                "Visit https://bugs.launchpad.net/maas-test/+filebug/%s "
+                "to file a bug and complete the maas-test reporting process."
+                % token)
+            ])
+
+    def test_saves_log_if_upload_fails(self):
+        blob = email.mime.multipart.MIMEMultipart()
+        mock_logger = mock.MagicMock()
+        mock_create_launchpad_blob = mock.MagicMock(return_value=blob)
+        mock_upload_blob = mock.MagicMock(return_value=None)
+        mock_write_test_results = mock.MagicMock()
+
+        self.patch(logging, 'info', mock_logger)
+        self.patch(report, 'create_launchpad_blob', mock_create_launchpad_blob)
+        self.patch(report, 'upload_blob', mock_upload_blob)
+        self.patch(report, 'write_test_results', mock_write_test_results)
+
+        results = self.getUniqueString()
+        now =datetime(2013, 12, 17, 7, 18, 0)
+        now_string = now.strftime("%Y-%m-%d_%H:%M:%S")
+        expected_filename = 'maas-test.%s.log' % now_string
+        report.report_test_results(results, now=now)
+        mock_write_test_results.assert_has_calls([
+            mock.call(expected_filename, results)])
+
+
+class TestWriteTestResults(testtools.TestCase):
+
+    def test_writes_results_to_file(self):
+        temp_dir = self.useFixture(TempDir()).path
+        results = self.getUniqueString()
+        log_file_name = self.getUniqueString()
+
+        report.write_test_results(log_file_name, results, temp_dir)
+
+        with open(os.path.join(temp_dir, log_file_name), 'r') as log_file:
+            self.assertEqual(results, log_file.read().strip())

=== modified file 'maastest/tests/test_utils.py'
--- maastest/tests/test_utils.py	2013-12-06 07:47:07 +0000
+++ maastest/tests/test_utils.py	2013-12-17 07:45:37 +0000
@@ -24,6 +24,7 @@
 from maastest.utils import binary_content
 import mock
 from six import text_type
+from StringIO import StringIO
 import testtools
 from testtools.matchers import (
     MatchesRegex,
@@ -364,3 +365,33 @@
         self.assertEqual(
             ["test_333", "test_111", "test_222"],
             [test._testMethodName for test in suite])
+
+
+class TestBufferingOutputStream(testtools.TestCase):
+
+    def test_writes_to_stream(self):
+        stream = StringIO()
+        buffering_stream = utils.BufferingOutputStream(stream)
+        output_string = self.getUniqueString()
+        buffering_stream.write(output_string)
+        self.assertEqual(output_string, stream.getvalue())
+
+    def test_buffers_output(self):
+        stream = StringIO()
+        buffering_stream = utils.BufferingOutputStream(stream)
+        buffering_stream.write(self.getUniqueString())
+        self.assertEqual(
+            stream.getvalue(), buffering_stream.buffer.getvalue())
+
+    def test_init_sets_values(self):
+        stream = StringIO()
+        buffering_stream = utils.BufferingOutputStream(stream)
+        self.assertEqual(stream, buffering_stream.stream)
+        self.assertIsInstance(buffering_stream.buffer, StringIO)
+
+    def test_attributes_looked_up_on_stream(self):
+        stream = StringIO()
+        some_string = self.getUniqueString()
+        stream.foo_bar_baz = mock.MagicMock(return_value=some_string)
+        buffering_stream = utils.BufferingOutputStream(stream)
+        self.assertEqual(some_string, buffering_stream.foo_bar_baz())

=== modified file 'maastest/utils.py'
--- maastest/utils.py	2013-12-06 07:47:07 +0000
+++ maastest/utils.py	2013-12-17 07:45:37 +0000
@@ -254,3 +254,18 @@
     """A wrapper around raw_input() that allows us to sanely mock it.
     """
     return input(message)
+
+
+class BufferingOutputStream:
+
+    def __init__(self, stream):
+        from StringIO import StringIO
+        self.buffer = StringIO()
+        self.stream = stream
+
+    def __getattr__(self, attr):
+        return getattr(self.stream, attr)
+
+    def write(self, stuff):
+        self.buffer.write(stuff)
+        self.stream.write(stuff)