Code review comment for lp://staging/~gmb/maas-test/add-reporting-follow-up

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