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

Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: 115
Merged at revision: 115
Proposed branch: lp:~gmb/maas-test/dont-report-successes
Merge into: lp:maas-test
Diff against target: 207 lines (+56/-65)
3 files modified
maastest/main.py (+5/-2)
maastest/report.py (+25/-23)
maastest/tests/test_report.py (+26/-40)
To merge this branch: bzr merge lp:~gmb/maas-test/dont-report-successes
Reviewer Review Type Date Requested Status
Julian Edwards (community) Disapprove
Raphaël Badin (community) Approve
Review via email: mp+201019@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

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
Revision history for this message
Graham Binns (gmb) wrote :

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.

Revision history for this message
Raphaël Badin (rvb) wrote :

> 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.

115. By Graham Binns

Added subject header.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
Revision history for this message
Graham Binns (gmb) wrote :

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.

Revision history for this message
Julian Edwards (julian-edwards) 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).

Revision history for this message
Graham Binns (gmb) 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.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
96 ConfiguredTestMAAS.tearDownClass()96 ConfiguredTestMAAS.tearDownClass()
97 raise97 raise
98 from maastest import report98 from maastest import report
99
99 encoded_results = (100 encoded_results = (
100 output_stream.cache.getvalue().encode('utf-8'))101 output_stream.cache.getvalue().encode('utf-8'))
101 if not args.no_reporting:102 if not args.no_reporting:
102 report.report_test_results(103 report.write_test_results(encoded_results)
103 encoded_results, log_only=args.log_results_only)104
104 if result.wasSuccessful():105 if result.wasSuccessful():
105 return RETURN_CODES.SUCCESS106 return RETURN_CODES.SUCCESS
106 else:107 else:
108 if not args.no_reporting and not args.log_results_only:
109 report.report_test_results(encoded_results)
107 return RETURN_CODES.TEST_FAILED110 return RETURN_CODES.TEST_FAILED
108111
109112
110113
=== 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 @@
60 launchpad_form_data['tags'] = 'maas-test-results'60 launchpad_form_data['tags'] = 'maas-test-results'
61 launchpad_form_data['private'] = 'yes'61 launchpad_form_data['private'] = 'yes'
62 launchpad_form_data['subscribers'] = "private-canonical-maas"62 launchpad_form_data['subscribers'] = "private-canonical-maas"
63 launchpad_form_data['subject'] = "maas-test failure"
6364
64 results_payload = make_file_payload(65 results_payload = make_file_payload(
65 'maas-test.log', test_results.encode('utf-8', 'replace'))66 'maas-test.log', test_results.encode('utf-8', 'replace'))
@@ -130,48 +131,49 @@
130 return token131 return token
131132
132133
133def write_test_results(log_file_name, results, log_dir=None):134def write_test_results(results, log_file_name=None, log_dir=None, now=None):
134 """Write maas-test results to a file.135 """Write maas-test results to a file.
135136
136 :param log_file_name: The filename to which to write the results.
137 :param results: The results from maas-test.137 :param results: The results from maas-test.
138 :param log_file_name: The filename to which to write the results
139 (used for testing only).
138 :param log_dir: The directory under which to create `log_file_name`.140 :param log_dir: The directory under which to create `log_file_name`.
141 :param now: The current date/time (used for testing only).
142 :type now: datetime.datetime.
139 """143 """
144 if now is None:
145 now = datetime.utcnow()
146
140 if log_dir is None:147 if log_dir is None:
141 log_dir = DEFAULT_LOG_DIR148 log_dir = DEFAULT_LOG_DIR
142 if not os.path.exists(log_dir):149 if not os.path.exists(log_dir):
143 os.makedirs(log_dir)150 os.makedirs(log_dir)
144151
152 if log_file_name is None:
153 log_file_name = (
154 "maas-test.%s.log" % now.strftime("%Y-%m-%d_%H:%M:%S"))
155
145 log_file_path = os.path.join(log_dir, log_file_name)156 log_file_path = os.path.join(log_dir, log_file_name)
146 with open(log_file_path, 'w') as log_file:157 with open(log_file_path, 'w') as log_file:
147 log_file.write(results)158 log_file.write(results)
159
148 logging.info("Test log saved to %s." % log_file_path)160 logging.info("Test log saved to %s." % log_file_path)
149161
150162
151def report_test_results(results, log_only=False, now=None):163def report_test_results(results):
152 """Handle the reporting of maas-test results.164 """Handle the reporting of maas-test results.
153165
154 :param results: The results to report.166 :param results: The results to report.
155 :param log_only: If True, write the results to a file but don't submit167 :param log_only: If True, write the results to a file but don't submit
156 them to Launchpad.168 them to Launchpad.
157 :param now: The current date/time (used for testing only).
158 :type now: datetime.datetime.
159 """169 """
160 if now is None:170 blob = create_launchpad_blob(results)
161 now = datetime.utcnow()171 blob_token = upload_blob(blob)
162172 if blob_token is not None:
163 log_file_name = (173 logging.info("Test results uploaded to Launchpad.")
164 "maas-test.%s.log" % now.strftime("%Y-%m-%d_%H:%M:%S"))174 logging.info(
165 write_test_results(log_file_name, results)175 "Visit https://bugs.launchpad.net/maas/+filebug/%s "
166176 "to file a bug and complete the maas-test reporting process."
167 if not log_only:177 % blob_token)
168 blob = create_launchpad_blob(results)178 else:
169 blob_token = upload_blob(blob)179 logging.info("Unable to store test results on Launchpad.")
170 if blob_token is not None:
171 logging.info("Test results uploaded to Launchpad.")
172 logging.info(
173 "Visit https://bugs.launchpad.net/maas/+filebug/%s "
174 "to file a bug and complete the maas-test reporting process."
175 % blob_token)
176 else:
177 logging.info("Unable to store test results on Launchpad.")
178180
=== 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 @@
65 blob = report.create_launchpad_blob(self.getUniqueString())65 blob = report.create_launchpad_blob(self.getUniqueString())
66 self.assertEqual("maas-test-results", blob['tags'])66 self.assertEqual("maas-test-results", blob['tags'])
6767
68 def test_sets_subject_header(self):
69 blob = report.create_launchpad_blob(self.getUniqueString())
70 self.assertEqual("maas-test failure", blob['subject'])
71
68 def test_sets_private_header(self):72 def test_sets_private_header(self):
69 blob = report.create_launchpad_blob(self.getUniqueString())73 blob = report.create_launchpad_blob(self.getUniqueString())
70 self.assertEqual("yes", blob['private'])74 self.assertEqual("yes", blob['private'])
@@ -227,43 +231,6 @@
227 % token)231 % token)
228 ])232 ])
229233
230 def test_saves_log(self):
231 blob = email.mime.multipart.MIMEMultipart()
232 mock_logger = mock.MagicMock()
233 mock_create_launchpad_blob = mock.MagicMock(return_value=blob)
234 mock_upload_blob = mock.MagicMock()
235 mock_write_test_results = mock.MagicMock()
236
237 self.patch(logging, 'info', mock_logger)
238 self.patch(report, 'create_launchpad_blob', mock_create_launchpad_blob)
239 self.patch(report, 'upload_blob', mock_upload_blob)
240 self.patch(report, 'write_test_results', mock_write_test_results)
241
242 results = self.getUniqueString()
243 now = datetime(2013, 12, 17, 7, 18, 0)
244 now_string = now.strftime("%Y-%m-%d_%H:%M:%S")
245 expected_filename = 'maas-test.%s.log' % now_string
246 report.report_test_results(results, now=now)
247 mock_write_test_results.assert_has_calls([
248 mock.call(expected_filename, results)])
249
250 def test_does_not_upload_blob_if_log_only_set(self):
251 mock_write_test_results = mock.MagicMock()
252 mock_upload_blob = mock.MagicMock()
253
254 self.patch(report, 'upload_blob', mock_upload_blob)
255 self.patch(report, 'write_test_results', mock_write_test_results)
256
257 results = self.getUniqueString()
258 now = datetime(2013, 12, 17, 7, 18, 0)
259 now_string = now.strftime("%Y-%m-%d_%H:%M:%S")
260 expected_filename = 'maas-test.%s.log' % now_string
261 report.report_test_results(results, log_only=True, now=now)
262 self.assertIn(
263 mock.call(expected_filename, results),
264 mock_write_test_results.mock_calls)
265 mock_upload_blob.assert_has_calls([])
266
267234
268class TestWriteTestResults(testtools.TestCase):235class TestWriteTestResults(testtools.TestCase):
269236
@@ -272,7 +239,7 @@
272 results = self.getUniqueString()239 results = self.getUniqueString()
273 log_file_name = self.getUniqueString()240 log_file_name = self.getUniqueString()
274241
275 report.write_test_results(log_file_name, results, temp_dir)242 report.write_test_results(results, log_file_name, temp_dir)
276243
277 with open(os.path.join(temp_dir, log_file_name), 'rb') as log_file:244 with open(os.path.join(temp_dir, log_file_name), 'rb') as log_file:
278 self.assertEqual(results, log_file.read().strip())245 self.assertEqual(results, log_file.read().strip())
@@ -280,9 +247,28 @@
280 def test_creates_log_dir(self):247 def test_creates_log_dir(self):
281 temp_dir = self.useFixture(TempDir()).path248 temp_dir = self.useFixture(TempDir()).path
282 results = self.getUniqueString()249 results = self.getUniqueString()
283 log_dir = os.path.join(temp_dir, 'logs')250 log_dir = os.path.join(temp_dir, self.getUniqueString())
284 log_file_name = self.getUniqueString()251 log_file_name = self.getUniqueString()
285252
286 self.assertFalse(os.path.exists(log_dir))253 self.assertFalse(os.path.exists(log_dir))
287 report.write_test_results(log_file_name, results, log_dir)254 report.write_test_results(results, log_file_name, log_dir)
288 self.assertTrue(os.path.exists(log_dir))255 self.assertTrue(os.path.exists(log_dir))
256
257 def test_saves_log_with_timestamp(self):
258 temp_dir = self.useFixture(TempDir()).path
259 results = self.getUniqueString()
260 mock_logger = mock.MagicMock()
261
262 self.patch(logging, 'info', mock_logger)
263
264 results = self.getUniqueString()
265 now = datetime(2013, 12, 17, 7, 18, 0)
266 now_string = now.strftime("%Y-%m-%d_%H:%M:%S")
267 expected_filename = 'maas-test.%s.log' % now_string
268 expected_file_path = os.path.join(temp_dir, expected_filename)
269 report.write_test_results(results, log_dir=temp_dir, now=now)
270 self.assertIn(
271 mock.call("Test log saved to %s." % expected_file_path),
272 mock_logger.mock_calls
273 )
274 self.assertTrue(os.path.exists(expected_file_path))

Subscribers

People subscribed via source and target branches