Merge lp:~bac/launchpad/bug-652280-pg-trans into lp:launchpad

Hi Brad,

Nice to see a branch with a solid cover letter again.

You found some interesting bugs in Storm's SQLObject compatibility layer while working on this: is_empty returns the inverse of what it should, and then __nonzero__ compounds that sin by inverting it again. I hope you'll file a bug about this.

I have no objections to the code overall. There are a few things in the test that I'd like to see fixed, but nothing to stop an approval:

 * You use lower_case_with_underscores for their helper methods, whereas our coding standard mandates sayItWithCapitals for methods (but methods only). The requirement may be going away though; I haven't kept track.

 * You pointed out that there's a redundant get_rendered_contents in the products test. To be removed.

 * You also pointed out that there's already a has_translatables on your context object, so no need to duplicate it in the view.

 * Why do you bother making "target" and "naked_translatable" properties instead of attributes that you initialize in setUp? I do appreciate the docstring but a comment would be fine as far as I'm concerned. You could scratch self.product, self.projectgroup, self.distro, and self.distroseries to make up for it; they would become unnecessary.

 * I like the orthogonal test setup, with a mixin for the test scenarios and leaf classes for the different alternate setups. But I find the inheritance graph a little confusing. I think it'd be more manageable if the actual test cases were all leaf classes, without test cases inheriting from other classes that will also run as separate test cases. I'd also put the layer specifications in those leaf classes, so that there's a clear separation between the classes that "go into" a test case and the individual test cases.

 * You have your own verification methods (verify_robots_are_blocked, verify_robots_are_not_blocked) that invoke other assertion methods. I normally avoid this myself because it hides the "where did it go wrong" information deeper in the traceback, but looking at this I quite like the way the naming documents the intent of the assertion.

 * Isn't testing for exactly "noindex,nofollow" more brittle than needed? Would "nofollow,noindex" be equally valid? If so, it'd be more robust to assertContentEqual on robots['content'].split(','). I'll admit it's far-fetched, but brittle tests have given us enough headaches to make us worry about these things.

 * Out of interest, why do you pass rootsite to create_initialized_view for distroseries but not for productseries?

Jeroen

On Oct 12, 2010, at 18:34 , Jeroen T. Vermeulen wrote:

> Review: Approve
> Hi Brad,

Thanks for the review Jeroen.

>
> Nice to see a branch with a solid cover letter again.

'bzr lp-send'

It's so easy I'm not sure why everyone doesn't do it.

>
> You found some interesting bugs in Storm's SQLObject compatibility layer while working on this: is_empty returns the inverse of what it should, and then __nonzero__ compounds that sin by inverting it again. I hope you'll file a bug about this.

Filed and fixed by you in record time! Thanks for the follow through.

>
> * You use lower_case_with_underscores for their helper methods, whereas our coding standard mandates sayItWithCapitals for methods (but methods only). The requirement may be going away though; I haven't kept track.

No, I was just not paying attention. Fixed.

>
> * You pointed out that there's a redundant get_rendered_contents in the products test. To be removed.

Gone.

>
> * You also pointed out that there's already a has_translatables on your context object, so no need to duplicate it in the view.

Gone along with the view tests. Added missing tests to doc/projectgroup.txt. (Renamed from project.txt.)

>
> * Why do you bother making "target" and "naked_translatable" properties instead of attributes that you initialize in setUp? I do appreciate the docstring but a comment would be fine as far as I'm concerned. You could scratch self.product, self.projectgroup, self.distro, and self.distroseries to make up for it; they would become unnecessary.

I did that before I converted the base class to a mixin to force the base to throw a NotImplementedError for those as I thought it was more explicit than a comment stating the contract. Overkill? Probably, so I have removed it.

>
> * I like the orthogonal test setup, with a mixin for the test scenarios and leaf classes for the different alternate setups. But I find the inheritance graph a little confusing. I think it'd be more manageable if the actual test cases were all leaf classes, without test cases inheriting from other classes that will also run as separate test cases. I'd also put the layer specifications in those leaf classes, so that there's a clear separation between the classes that "go into" a test case and the individual test cases.

OK, changed to all extend BrowserTestCase and the mixin directly. I disagree with setting the layer on each one, though, as it is the same. That just seems redundant and silly.

>
> * You have your own verification methods (verify_robots_are_blocked, verify_robots_are_not_blocked) that invoke other assertion methods. I normally avoid this myself because it hides the "where did it go wrong" information deeper in the traceback, but looking at this I quite like the way the naming documents the intent of the assertion.
>
> * Isn't testing for exactly "noindex,nofollow" more brittle than needed? Would "nofollow,noindex" be equally valid? If so, it'd be more robust to assertContentEqual on robots['content'].split(','). I'll admit it's far-fetched, but brittle tests have given us enough headaches to make us worry about these things.

Yes, that probably is too brittle. I'll ...

This looks just fine to me. Nice to see how much boilerplate you were able to strip off the tests. The layers thing was just a suggestion; I'm fine with your choice.

Hi Curtis,

This branch includes a lot of good stuff...but it only addresses one part of the original bug. There was disagreement between you and Danilo about what UI changes needed to be done. What are your current thoughts?

The Translations team is also changing these pages. There is not need to change something that the team will remove or alter in a few weeks anyway.

I'm very happy with one test:

169 + def test_service_status_controls_robots(self):
170 + self.verifyRobotsAreBlocked(ServiceUsage.UNKNOWN)
173 + self.verifyRobotsAreBlocked(ServiceUsage.EXTERNAL)
176 + self.verifyRobotsAreBlocked(ServiceUsage.NOT_APPLICABLE)
179 + self.verifyRobotsNotBlocked(ServiceUsage.LAUNCHPAD)

or however you wish to spell it; using cachedproperty should be ok if you want to, but be sure to do browser.open() after you change the service usage. How many seconds difference does the cachedproperty make (perhaps we need a bug on the performance of canonical_url?)