Merge lp:~gmb/maas-test/dont-report-successes into lp:maas-test

[Graham Binns]

Only report to Launchpad on failure; always log on success (unless told otherwise).

This branch changes the way that reporting works so that we only report to Launchpad on failure, but always store the test results unless told not to.

[Raphaël Badin (rvb)]

Looks good.

On the one hand, I think that the code is better structured this way (having the control of whether or not things are logged in the main execution flow and all the actual work done in helper methods) but on the other hand, it means part of the code escaped in non-tested territory. Oh well, this is still probably for the best.

[0]

96 + "Visit https://bugs.launchpad.net/maas/+filebug/%s "
97 + "to file a bug and complete the maas-test reporting process."

Btw, this is unrelated to this change but I was wondering if we could pre-fill the title of the bug somehow. Do you know if it's possible? If it is, it would be nice because —unless the user changes it of course— it would enable us to quickly spot if a bug has been submitted because of a failed maas-test run; it would be even better if we could attach a tag to it.

review: Approve

[Graham Binns (gmb)]

On 9 January 2014 15:27, Raphaël Badin <email address hidden> wrote:
> Btw, this is unrelated to this change but I was wondering if we could pre-fill the title of the bug somehow. Do you know if it's possible? If it is, it would be nice because —unless the user changes it of course— it would enable us to quickly spot if a bug has been submitted because of a failed maas-test run; it would be even better if we could attach a tag to it.

We can — and we did, in my initial branch, but we dropped it IIRC. We
already tag it with "maas-test-results". Do you think just pre-filling
it with "maas-test failure" would be sufficient? We can always update
the title to be more specific later.

[Raphaël Badin (rvb)]

> We can — and we did, in my initial branch, but we dropped it IIRC. We
> already tag it with "maas-test-results". Do you think just pre-filling
> it with "maas-test failure" would be sufficient? We can always update
> the title to be more specific later.

Yeah, "maas-test failure" sounds like a good start.

[lp:~gmb/maas-test/dont-report-successes]

115. By Graham Binns on 2014-01-09

Added subject header.

[Julian Edwards (julian-edwards)]

While I appreciate we don't necessarily want to file bugs for successes, the point of maas-test is that we DO report successes *somewhere*. Since we don't have somewhere else, I am afraid this branch should be reverted.

review: Disapprove

[Graham Binns (gmb)]

Warning: early-morning shambolic rambling ahead:

On 10 January 2014 00:57, Julian Edwards <email address hidden> wrote:
> Review: Disapprove
>
> While I appreciate we don't necessarily want to file bugs for
> successes, the point of maas-test is that we DO report successes
> *somewhere*. Since we don't have somewhere else, I am afraid this
> branch should be reverted.

Okay, I can see that recording successes has some value — maybe more
value than I realise beyond the "look at everything that works with
MAAS" marketing standpoint. But do we _really_ want to have every
success captured?

I suppose we could file them as "Fix Released" :). (Assuming LP lets you
do that).

My argument's a little fuzzy here — hey, it's 07:24 and I haven't
finished my first cup of tea yet — but here goes:

 1. We want to record all test runs, be they pass or failure.
 2. However, passes all look the same
 3. That means that we'd end up with a lot of duplicate bugs
 4. Worse, de-duping them will be really hard because there's nothing
    unique about the reports. (In fact this applies to failures as well
    as successes — imagine if 10 companies try maas-test on similar but
    subtly different hardware).
 5. Actually, then, my argument isn't "we shouldn't report successes" at
    all; in fact, my argument is "maas-test reports should carry some
    identifying information as standard so that de-duping is easier".
 6. If that were true, I'd be fine with reverting this; at the moment
    I'm -0 at the most.

[Julian Edwards (julian-edwards)]

We do need to report success, so some way of IDing the test run on a
hardware basis would be the best way forwards here (and then dupe on that).

[Graham Binns (gmb)]

On 10 January 2014 09:27, Julian Edwards <email address hidden> wrote:
> We do need to report success, so some way of IDing the test run on a
> hardware basis would be the best way forwards here (and then dupe on that).

+1. I'll revert this branch for starters.

[Julian Edwards (julian-edwards)]

On 10/01/14 19:42, Graham Binns wrote:
> On 10 January 2014 09:27, Julian Edwards <email address hidden> wrote:
>> We do need to report success, so some way of IDing the test run on a
>> hardware basis would be the best way forwards here (and then dupe on that).
>
> +1. I'll revert this branch for starters.
>

Thanks. Sorry I wasn't around to put you off in the first place, I keep
assuming everyone knows everything... and you know what they say about
assumptions.

=== modified file 'maastest/main.py'
--- maastest/main.py	2014-01-09 11:35:01 +0000
+++ maastest/main.py	2014-01-09 16:05:20 +0000
@@ -96,14 +96,17 @@
         ConfiguredTestMAAS.tearDownClass()
         raise
     from maastest import report
+
     encoded_results = (
         output_stream.cache.getvalue().encode('utf-8'))
     if not args.no_reporting:
-        report.report_test_results(
-            encoded_results, log_only=args.log_results_only)
+        report.write_test_results(encoded_results)
+
     if result.wasSuccessful():
         return RETURN_CODES.SUCCESS
     else:
+        if not args.no_reporting and not args.log_results_only:
+            report.report_test_results(encoded_results)
         return RETURN_CODES.TEST_FAILED
 
 

=== modified file 'maastest/report.py'
--- maastest/report.py	2014-01-09 12:12:32 +0000
+++ maastest/report.py	2014-01-09 16:05:20 +0000
@@ -60,6 +60,7 @@
     launchpad_form_data['tags'] = 'maas-test-results'
     launchpad_form_data['private'] = 'yes'
     launchpad_form_data['subscribers'] = "private-canonical-maas"
+    launchpad_form_data['subject'] = "maas-test failure"
 
     results_payload = make_file_payload(
         'maas-test.log', test_results.encode('utf-8', 'replace'))
@@ -130,48 +131,49 @@
         return token
 
 
-def write_test_results(log_file_name, results, log_dir=None):
+def write_test_results(results, log_file_name=None, log_dir=None, now=None):
     """Write maas-test results to a file.
 
-    :param log_file_name: The filename to which to write the results.
     :param results: The results from maas-test.
+    :param log_file_name: The filename to which to write the results
+        (used for testing only).
     :param log_dir: The directory under which to create `log_file_name`.
+    :param now: The current date/time (used for testing only).
+    :type now: datetime.datetime.
     """
+    if now is None:
+        now = datetime.utcnow()
+
     if log_dir is None:
         log_dir = DEFAULT_LOG_DIR
     if not os.path.exists(log_dir):
         os.makedirs(log_dir)
 
+    if log_file_name is None:
+        log_file_name = (
+            "maas-test.%s.log" % now.strftime("%Y-%m-%d_%H:%M:%S"))
+
     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, log_only=False, now=None):
+def report_test_results(results):
     """Handle the reporting of maas-test results.
 
     :param results: The results to report.
     :param log_only: If True, write the results to a file but don't submit
         them to Launchpad.
-    :param now: The current date/time (used for testing only).
-    :type now: datetime.datetime.
     """
-    if now is None:
-        now = datetime.utcnow()
-
-    log_file_name = (
-        "maas-test.%s.log" % now.strftime("%Y-%m-%d_%H:%M:%S"))
-    write_test_results(log_file_name, results)
-
-    if not log_only:
-        blob = create_launchpad_blob(results)
-        blob_token = upload_blob(blob)
-        if blob_token is not None:
-            logging.info("Test results uploaded to Launchpad.")
-            logging.info(
-                "Visit https://bugs.launchpad.net/maas/+filebug/%s "
-                "to file a bug and complete the maas-test reporting process."
-                % blob_token)
-        else:
-            logging.info("Unable to store test results on Launchpad.")
+    blob = create_launchpad_blob(results)
+    blob_token = upload_blob(blob)
+    if blob_token is not None:
+        logging.info("Test results uploaded to Launchpad.")
+        logging.info(
+            "Visit https://bugs.launchpad.net/maas/+filebug/%s "
+            "to file a bug and complete the maas-test reporting process."
+            % blob_token)
+    else:
+        logging.info("Unable to store test results on Launchpad.")

=== modified file 'maastest/tests/test_report.py'
--- maastest/tests/test_report.py	2014-01-09 12:12:32 +0000
+++ maastest/tests/test_report.py	2014-01-09 16:05:20 +0000
@@ -65,6 +65,10 @@
         blob = report.create_launchpad_blob(self.getUniqueString())
         self.assertEqual("maas-test-results", blob['tags'])
 
+    def test_sets_subject_header(self):
+        blob = report.create_launchpad_blob(self.getUniqueString())
+        self.assertEqual("maas-test failure", blob['subject'])
+
     def test_sets_private_header(self):
         blob = report.create_launchpad_blob(self.getUniqueString())
         self.assertEqual("yes", blob['private'])
@@ -227,43 +231,6 @@
                 % token)
             ])
 
-    def test_saves_log(self):
-        blob = email.mime.multipart.MIMEMultipart()
-        mock_logger = mock.MagicMock()
-        mock_create_launchpad_blob = mock.MagicMock(return_value=blob)
-        mock_upload_blob = mock.MagicMock()
-        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)])
-
-    def test_does_not_upload_blob_if_log_only_set(self):
-        mock_write_test_results = mock.MagicMock()
-        mock_upload_blob = mock.MagicMock()
-
-        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, log_only=True, now=now)
-        self.assertIn(
-            mock.call(expected_filename, results),
-            mock_write_test_results.mock_calls)
-        mock_upload_blob.assert_has_calls([])
-
 
 class TestWriteTestResults(testtools.TestCase):
 
@@ -272,7 +239,7 @@
         results = self.getUniqueString()
         log_file_name = self.getUniqueString()
 
-        report.write_test_results(log_file_name, results, temp_dir)
+        report.write_test_results(results, log_file_name, temp_dir)
 
         with open(os.path.join(temp_dir, log_file_name), 'rb') as log_file:
             self.assertEqual(results, log_file.read().strip())
@@ -280,9 +247,28 @@
     def test_creates_log_dir(self):
         temp_dir = self.useFixture(TempDir()).path
         results = self.getUniqueString()
-        log_dir = os.path.join(temp_dir, 'logs')
+        log_dir = os.path.join(temp_dir, self.getUniqueString())
         log_file_name = self.getUniqueString()
 
         self.assertFalse(os.path.exists(log_dir))
-        report.write_test_results(log_file_name, results, log_dir)
+        report.write_test_results(results, log_file_name, log_dir)
         self.assertTrue(os.path.exists(log_dir))
+
+    def test_saves_log_with_timestamp(self):
+        temp_dir = self.useFixture(TempDir()).path
+        results = self.getUniqueString()
+        mock_logger = mock.MagicMock()
+
+        self.patch(logging, 'info', mock_logger)
+
+        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
+        expected_file_path = os.path.join(temp_dir, expected_filename)
+        report.write_test_results(results, log_dir=temp_dir, now=now)
+        self.assertIn(
+            mock.call("Test log saved to %s." % expected_file_path),
+            mock_logger.mock_calls
+            )
+        self.assertTrue(os.path.exists(expected_file_path))