Code review comment for lp:~allenap/maas/more-better-things--bug-1389007

Revision history for this message
Gavin Panella (allenap) wrote :

> Typo: "This contain the".

Well spotted, thanks. Fixed.

>
> .
>
> If you're going to include Twisted code here, it may not be valid to
> say "See LICENSE for details." You may need to include their licence
> file under a recognisable name, and make the copyright header refer to
> it as such. Also, if you're creating a derived work with substantial
> changes, I guess it needs our copyright notice as well.

I've pulled Twisted's LICENSE file in now, as LICENSE.Twisted, and
updated the headers in those files. Canonical is already listed in
LICENSE.Twisted so I think we can probably leave it as is.

>
> .
>
> AMPTestCase (not AMP32TestCase?) and everything in it needs
> docstrings.
>
> Although to be honest, I'd prefer not to have yet another test-case
> class in the first place! I see very little in there that actually
> needs to be in a class: shortDescription might be generic or it could
> be in a fixture. setUp *is* a fixture. getLoggedFailures could be in
> MAASTestCase. And assertWarns is a generic matcher disguised as a
> specific helper which makes non- transparent decisions about whether
> the test should continue after failure.

shortDescription() is a unittest method that gets called before the test
runs, but now that I've rebased on MAASTestCase instead of Trial's
TestCase it can go.

I've added docstrings which explain the other functions, and justify
their continued existence. I could spend time on getting rid of
AMP32TestCase, but the pay off doesn't seem worth it right now. I'm
biased, but I think it's already an improvement over the tests that work
with Trial.

>
> .
>
> In AMPTestCase.assertWarns, if you know you've got a list of one item
> and you want the item, prefer extracting it using pattern-matching
> ("[item] = mylist") over indexing ("mylist[0]").

Done.

Thanks!

« Back to merge proposal