launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

> In src/maastesting/tests/test_httpd.py:
> 
> 736     +    def test_use(self):
> 737     +        filename = "setup.py"
> 738     +        self.assertThat(filename, FileExists())
> 739     +        with HTTPServerFixture() as httpd:
> 740     +            url = urljoin(httpd.url, filename)
> 741     +            with closing(urlopen(url)) as http_in:
> 742     +                with open(filename, "rb") as file_in:
> 743     +                    self.assertEqual(
> 744     +                        file_in.read(), http_in.read(),
> 745     +                        "The content of %s differs from %s." % (
> 746     +                            url, filename))
> 
> Lots more indentation.  I think this would be easier on the eyes if you only
> nested the extraction of the file contents, and moved the assertion back to
> the method's base indentation level.

Done.

> 
> In src/maastesting/tests/test_saucelabs.py, test_setUp_and_cleanUp
> tests the exact letter-for-letter contents of the command line that
> the fixture will run.  Isn't that unnecessarily brittle?  When the
> command line changes, what will the accompanying test failure tell
> us besides the change itself?

Yeah, I was concerned about that too, but I want to ensure that the
correct arguments are being passed out. I think we can live with the
brittleness.

> 
> Also in that file, one_retry iterates once, but does that count as a
> retry?  Would a name like no_retries or one_attempt cover it more
> accurately?

Good point. I've gone for one_attempt.

Thanks for the review!

-- 
https://code.launchpad.net/~allenap/maas/try-out-saucelabs/+merge/106217
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/try-out-saucelabs into lp:maas.