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

Delightful, thanks. A few notes as (almost) always.

.

It looks like a torn-off snippet of text landed inadvertently at the top of docs/man/maas-test.8.rst. A more complete version is about a hundred lines further down.

Where, by the way, it says "By defaults" where I would have expected "By default." And I notice you write "disk" in the documentation but "disc" in the MP — wasn't the last decision in our former Launchpad team to use British English? At any rate, "file" may avoid some unnecessary specificity.

.

In maastest/parser.py, the help texts for the new options are a bit long. Given that the manpage provides more extensive information anyway, can you think of more concise descriptions here? How about respectively “Log results only, do not store them or submit them to Launchpad” and “Store results, but do not submit them to Launchpad”?

.

It's a shame that report_test_results() doesn't have more information about why creating a blob might fail. Also, shouldn't that timestamp be either (a) always in UTC, or (b) be logged with a timezone?

.

Typo: “def test_doest_upload_blob_if_log_only_set(self):” has a “doest” where you probably meant either “doesnt” or, at a stretch, “doth” for 3rd person instead of 2nd.

Consider saying "does_not" instead.

.

In that same test, a spacing typo:

        now =datetime(2013, 12, 17, 7, 18, 0)

Did you run "make lint"?

.

Just below that, you use mock.MagicMock.assert_has_calls(). Mock's design makes that dangerous. If you get everything right, the test passes. If you type it wrong, the test passes. If the assertion doesn't hold but you type it right, the test fails. If the assertion doesn't hold and you type it wrong, the test passes.

It may not seem like much of an issue because in TDD you watch the test fail before you make it pass, but code changes, conditions change, and you want to be sure the test doesn't become a broken alarm light. It's only a tiny change to self.assertIn(call(...), my_mock.mock_calls) which is less prone to false negatives.

.

Typo in the docstring for BufferingOutputStream.__getattr__:

        """Pass-through for any attribute ther than write().

Also, it would be nice to stick to the Python convention of documenting in the imperative: "Pass through any attribute other than `write`."

.

Still in BufferingOutputStream, why is the StringIO import not at the top of the file?

.

Finally, it's not clear to me that BufferingOutputStream actually buffers. Everything about its names and documentation gives me the impression that it collects data in a buffer and only writes it to the stream when the buffer overflows or when the buffer is closed. But the code looks more like a "tee"!

Could you clear that up?

Jeroen

> Delightful, thanks. A few notes as (almost) always.
>
> .
>
> It looks like a torn-off snippet of text landed inadvertently at the top of
> docs/man/maas-test.8.rst. A more complete version is about a hundred lines
> further down.
>
> Where, by the way, it says "By defaults" where I would have expected "By
> default." And I notice you write "disk" in the documentation but "disc" in
> the MP — wasn't the last decision in our former Launchpad team to use British
> English? At any rate, "file" may avoid some unnecessary specificity.

Gaah, spelling. It's rare that I type "disc" these days, I must have had a giddy moment. Yes, let's go with "file"

> .
>
> In maastest/parser.py, the help texts for the new options are a bit long.
> Given that the manpage provides more extensive information anyway, can you
> think of more concise descriptions here? How about respectively “Log results
> only, do not store them or submit them to Launchpad” and “Store results, but
> do not submit them to Launchpad”?

Yep, those sound fine.

> .
>
> It's a shame that report_test_results() doesn't have more information about
> why creating a blob might fail.

Yeah, I'm not too hot about that, but if there's a failure it's logged by upload_blob() so at least we have something. I found in my experimentations that uploading blobs to Launchpad is at best ill-documented, at worst positively Kafk-esque, so there's not actually that much information to be had in the first place. (Worst yet is the fact that for all our hoop-jumping here, there's every possibility that the LP side of blob processing could change in the future and it would just start silently dropping our now non-compliant blobs).

> Also, shouldn't that timestamp be either (a)
> always in UTC, or (b) be logged with a timezone?

For the sake of my sanity, I've changed it to use datetime.utcnow() instead of datetime.now(), but I'm not sure how to test that. Any ideas?

> Typo: “def test_doest_upload_blob_if_log_only_set(self):” has a “doest” where
> you probably meant either “doesnt” or, at a stretch, “doth” for 3rd person
> instead of 2nd.
>
> Consider saying "does_not" instead.

Fixed.

>
> In that same test, a spacing typo:
>
> now =datetime(2013, 12, 17, 7, 18, 0)
>
> Did you run "make lint"?

Whoops. No, I forgot it's not run automatically.

> .
>
> Just below that, you use mock.MagicMock.assert_has_calls(). Mock's design
> makes that dangerous. If you get everything right, the test passes. If you
> type it wrong, the test passes. If the assertion doesn't hold but you type it
> right, the test fails. If the assertion doesn't hold and you type it wrong,
> the test passes.
>
> It may not seem like much of an issue because in TDD you watch the test fail
> before you make it pass, but code changes, conditions change, and you want to
> be sure the test doesn't become a broken alarm light. It's only a tiny change
> to self.assertIn(call(...), my_mock.mock_calls) which is less prone to false
> negatives.
>

Oy. Thanks for the tip.

>
> Typo in the docstring for BufferingOutputStream.__getattr__:
>
> """Pass-through for any attribute ther than write().
>
> Also, it would ...