Code review comment for lp:~mwhudson/launchpad/vostok-main-template

On Thu, 29 Jul 2010 23:38:41 -0000, Tim Penhey <email address hidden> wrote:
> Review: Needs Information
> > class VostokTestRequest(VostokRequestMixin, LaunchpadTestRequest):
> > pass
>
> Instead of pass, how about a docstring?

OK.

> The browser tests are missing docstrings too. I know they are (most likely)
> boring, but should be there anyway.

Added.

> > def test_distributions(self):
> > # VostokRootView.distributions is an iterable of all registered
> > # distributions.
> > root_view = self.view()
>
> This likes you are actually doing
>
> VostokRootView.__call__
>
> Is that what you want? I wouldn't have thought so.

view is a method, not a property.

> Are you familiar at all with the new lp.testing.BrowserTestCase?
> It might be better than manually creating the view, initializing it, and
> rendering it.

The basic reason I didn't use testbrowser was that I wanted to have the
view object around so I can check the list matches up with what's in the
template -- the idea being to test the template. I agree it's a bit
ugly.

> VostokBrowserRequest could benefit from a docstring rather than pass too.

I sprinkled a few docstrings around.

> I think that your main-template.pt macro should be a valid HTML file rather
> than just <h1> and <div>. I'm pretty sure that most of the HTML in root.pt is
> discarded given that it is using the master macro.

You were right. Fixed.

Cheers,
mwh