Launchpad itself

Code review comment for lp:~mbp/launchpad/flags-gui

flags-gui
Merge into devel

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-09-29:

Hi Martin,

Following up in two parts. This is part one. I, too, will skip the parts I agree with and which seem to require no further action.

> > 1 === added directory 'lib/lp/services/features/browser'
> > 2 === added file 'lib/lp/services/features/browser/__init__.py'
> > 3 --- lib/lp/services/features/browser/__init__.py 1970-01-01
> 00:00:00 +0000
> > 4 +++ lib/lp/services/features/browser/__init__.py 2010-09-27
> 08:17:44 +0000
> > 5 @@ -0,0 +1,45 @@
> > 6 +# Copyright 2010 Canonical Ltd. This software is licensed under the
> > 7 +# GNU Affero General Public License version 3 (see the file
> LICENSE).
> >
> > I don't much care for putting code in __init__.py. I can see that it keeps
> the imports shorter, but it's unusual enough that it may be easy to miss.
>
> OK. Can I make it just browser.py, or should I have a submodule with
> a named file within it?

If a nontrivial __init__.py is already a normal thing to write, then there's no reason to worry about it here after all. So it's fine to leave it as it is.

> > 91 === added file
> 'lib/lp/services/features/browser/tests/test_feature_editor.py'
> > 92 --- lib/lp/services/features/browser/tests/test_feature_editor.py
> 1970-01-01 00:00:00 +0000
> > 93 +++ lib/lp/services/features/browser/tests/test_feature_editor.py
> 2010-09-27 08:17:44 +0000
> >
> > 130 +class TestFeatureControlPage(BrowserTestCase):
> > 131 +
> > 132 + layer = DatabaseFunctionalLayer
> > 133 +
> > 134 + def getUserBrowserAsTeamMember(self, url, teams):
> > 135 + """Make a TestBrowser authenticated as a team member.
> > 136 +
> > 137 + :param teams: List of teams to add the new user to.
> > 138 + """
> >
> > This method dithers between being a test setup helper and a test
> verification helper. Things end up muddled: when you expect
> getUserBrowserAsTeamMember to raise an authorization error, you're making an
> assertion about the "open the page" part but the code makes it look as if
> you're talking about the "add user to teams" part instead.
>
> Hm, so perhaps (if it's possible) it would be clearer to write something like:
>
> browser = getUserBrowserAsTeamMember(teams)
> self.assertRaises(Unauthorized, browser.open(url))

Yes, exactly. And AFAICS that should work.

> > 152 + def getFeaturePageBrowserAsAdmin(self):
> > 153 + url = self.getFeatureRulesURL()
> > 154 + admin_team = getUtility(ILaunchpadCelebrities).admin
> > 155 + return self.getUserBrowserAsTeamMember(url, [admin_team])
> >
> > It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."
> Have you considered creating one?
>
> Right, that's bug 646563, but this branch is getting big enough already.

Very well.

> > 165 + def test_feature_page_from_database(self):
> > 166 + addFeatureFlagRules([
> > 167 + ('default', 'ui.icing', u'3.0', 100),
> > 168 + ('beta_user', 'ui.icing', u'4.0', 300),
> > 169 + ])
> > 170 + browser = self.getFeaturePageBrowserAsAdmin()
> > 171 + textarea = browser.getControl(name="feature_rules")
> > 172 + self.assertThat(textarea.value.replace('\r', ''),
> Equals("""\
> > 173 +ui.icing\tbeta_user\t300\t4.0
> > 174 +ui.icing\tdefault\t100\t3.0
> > 175 +"""))
> >
> > Try BrowserTestCase.assertTextMatchesExpressionIgnoreWhitespace instead.
> It's not great, so feel free to improve it and help everyone. (Also, if you
> find yourself dedenting manually like this in the future, consider using
> textwrap.dedent).
>
> Are we actually better off ignoring whitespace? Perhaps so. If I was
> going to change it, I'd change it into a testtools Matcher.

I think ignoring whitespace would be a fine thing to do, assuming that a separate test verifies that the whitespace conforms to expectations.

A Matcher would be very nice here.

> > If you do activate the commented-out code, please use one of the accepted
> line-breaking styles for the assertion. If not, remove this test altogether
> and file a separate bug.
>
> iow with the first line break immediately after the opening parenthesis.

Precisely.

> > 322 === modified file 'lib/lp/services/features/model.py'
> > 323 --- lib/lp/services/features/model.py 2010-08-20 20:31:18 +0000
> > 324 +++ lib/lp/services/features/model.py 2010-09-27 08:17:44 +0000
> >
> > 347 +def addFeatureFlagRules(rule_list):
> >
> > A standalone function should follow the lower_case_with_underscores naming
> convention.
>
> stand_alone_function vs objectMethod? omg.

Indeed. Since PEP8 prescribes lower_case_with_underscores, we may start accepting that in both cases. But dromedaryCase for stand-alone functions is not OK either way.

> > 348 + """Add rules in to the database; intended for use in testing.
> >
> > Then shouldn't it live in lp.testing? Call it makeFeatureFlagRule and it'd
> fit perfectly in the LaunchpadObjectFactory.
>
> Actually I think for most testing, we want to just change the
> featurecontroller, not actually change the database. And perhaps it
> should be a test fixture not on that monolith class?

I could imagine a need for both "create a rule in the database" and "create a rule in whatever rules source I'm currently using." Which you want this function for is up to you, and I defer to your judgment.

> > 360 === added file 'lib/lp/services/features/rulesource.py'
> > 361 --- lib/lp/services/features/rulesource.py 1970-01-01 00:00:00
> +0000
> > 362 +++ lib/lp/services/features/rulesource.py 2010-09-27 08:17:44
> +0000
> > 363 @@ -0,0 +1,90 @@
> > 364 +# Copyright 2010 Canonical Ltd. This software is licensed under
> the
> > 365 +# GNU Affero General Public License version 3 (see the file
> LICENSE).
> > 366 +
> > 367 +"""Returns rules defining which features are active"""
> >
> > This looks like a function docstring. Could you make it applicable to the
> module as a whole?
>
> Do you mean, rephrase it so it talks about what kind of thing the
> module contains, rather than what the class does.

Right.

> > I'm not sure about the design of this module. Separating out the storage
> layer is obviously very useful, and it looks as if you could do with a
> setAllRules in NullFeatureRuleSource as well. But the testing code and
> production-usable code live very close together here. Wouldn't it be more
> appropriate (and consistent with our existing codebase) to have a functional
> FeatureRuleSource for production, plus a FakeRuleSource in lp.testing that
> overrides the two storage methods?
>
> If the consistent pattern is that implementations only for testing use
> go into lp.testing then I'll move it there.

I think it is, yes. A right little fakes library is forming there. Before that, we usually put most such things in the individual tests. (And, as you're probably about to say, repeated them a lot).

More to follow in part two!

Hi Martin,

Following up in two parts.  This is part one.  I, too, will skip the parts I agree with and which seem to require no further action.

> >  1     === added directory 'lib/lp/services/features/browser'
> >  2     === added file 'lib/lp/services/features/browser/__init__.py'
> >  3     --- lib/lp/services/features/browser/__init__.py        1970-01-01
> 00:00:00 +0000
> >  4     +++ lib/lp/services/features/browser/__init__.py        2010-09-27
> 08:17:44 +0000
> >  5     @@ -0,0 +1,45 @@
> >  6     +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> >  7     +# GNU Affero General Public License version 3 (see the file
> LICENSE).
> >
> > I don't much care for putting code in __init__.py.  I can see that it keeps
> the imports shorter, but it's unusual enough that it may be easy to miss.
> 
> OK.  Can I make it just browser.py, or should I have a submodule with
> a named file within it?

If a nontrivial __init__.py is already a normal thing to write, then there's no reason to worry about it here after all.  So it's fine to leave it as it is.

> >  91     === added file
> 'lib/lp/services/features/browser/tests/test_feature_editor.py'
> >  92     --- lib/lp/services/features/browser/tests/test_feature_editor.py
> 1970-01-01  00:00:00 +0000
> >  93     +++ lib/lp/services/features/browser/tests/test_feature_editor.py
> 2010-09-27 08:17:44 +0000
> >
> > 130     +class TestFeatureControlPage(BrowserTestCase):
> > 131     +
> > 132     +    layer = DatabaseFunctionalLayer
> > 133     +
> > 134     +    def getUserBrowserAsTeamMember(self, url, teams):
> > 135     +        """Make a TestBrowser authenticated as a team member.
> > 136     +
> > 137     +        :param teams: List of teams to add the new user to.
> > 138     +        """
> >
> > This method dithers between being a test setup helper and a test
> verification helper.  Things end up muddled: when you expect
> getUserBrowserAsTeamMember to raise an authorization error, you're making an
> assertion about the "open the page" part but the code makes it look as if
> you're talking about the "add user to teams" part instead.
> 
> Hm, so perhaps (if it's possible) it would be clearer to write something like:
> 
>   browser = getUserBrowserAsTeamMember(teams)
>   self.assertRaises(Unauthorized, browser.open(url))

Yes, exactly.  And AFAICS that should work.

> > 152     +    def getFeaturePageBrowserAsAdmin(self):
> > 153     +        url = self.getFeatureRulesURL()
> > 154     +        admin_team = getUtility(ILaunchpadCelebrities).admin
> > 155     +        return self.getUserBrowserAsTeamMember(url, [admin_team])
> >
> > It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."
> Have you considered creating one?
> 
> Right, that's bug 646563, but this branch is getting big enough already.

Very well.

> > 165     +    def test_feature_page_from_database(self):
> > 166     +        addFeatureFlagRules([
> > 167     +            ('default', 'ui.icing', u'3.0', 100),
> > 168     +            ('beta_user', 'ui.icing', u'4.0', 300),
> > 169     +            ])
> > 170     +        browser = self.getFeaturePageBrowserAsAdmin()
> > 171     +        textarea = browser.getControl(name="feature_rules")
> > 172     +        self.assertThat(textarea.value.replace('\r', ''),
> Equals("""\
> > 173     +ui.icing\tbeta_user\t300\t4.0
> > 174     +ui.icing\tdefault\t100\t3.0
> > 175     +"""))
> >
> > Try BrowserTestCase.assertTextMatchesExpressionIgnoreWhitespace instead.
> It's not great, so feel free to improve it and help everyone.  (Also, if you
> find yourself dedenting manually like this in the future, consider using
> textwrap.dedent).
> 
> Are we actually better off ignoring whitespace?  Perhaps so.  If I was
> going to change it, I'd change it into a testtools Matcher.

I think ignoring whitespace would be a fine thing to do, assuming that a separate test verifies that the whitespace conforms to expectations.

A Matcher would be very nice here.

> > If you do activate the commented-out code, please use one of the accepted
> line-breaking styles for the assertion.  If not, remove this test altogether
> and file a separate bug.
> 
> iow with the first line break immediately after the opening parenthesis.

Precisely.

> > 322     === modified file 'lib/lp/services/features/model.py'
> > 323     --- lib/lp/services/features/model.py   2010-08-20 20:31:18 +0000
> > 324     +++ lib/lp/services/features/model.py   2010-09-27 08:17:44 +0000
> >
> > 347     +def addFeatureFlagRules(rule_list):
> >
> > A standalone function should follow the lower_case_with_underscores naming
> convention.
> 
> stand_alone_function vs objectMethod? omg.

Indeed.  Since PEP8 prescribes lower_case_with_underscores, we may start accepting that in both cases.  But dromedaryCase for stand-alone functions is not OK either way.

> > 348     +    """Add rules in to the database; intended for use in testing.
> >
> > Then shouldn't it live in lp.testing?  Call it makeFeatureFlagRule and it'd
> fit perfectly in the LaunchpadObjectFactory.
> 
> Actually I think for most testing, we want to just change the
> featurecontroller, not actually change the database.  And perhaps it
> should be a test fixture not on that monolith class?

I could imagine a need for both "create a rule in the database" and "create a rule in whatever rules source I'm currently using."  Which you want this function for is up to you, and I defer to your judgment.

> > 360     === added file 'lib/lp/services/features/rulesource.py'
> > 361     --- lib/lp/services/features/rulesource.py      1970-01-01 00:00:00
> +0000
> > 362     +++ lib/lp/services/features/rulesource.py      2010-09-27 08:17:44
> +0000
> > 363     @@ -0,0 +1,90 @@
> > 364     +# Copyright 2010 Canonical Ltd.  This software is licensed under
> the
> > 365     +# GNU Affero General Public License version 3 (see the file
> LICENSE).
> > 366     +
> > 367     +"""Returns rules defining which features are active"""
> >
> > This looks like a function docstring.  Could you make it applicable to the
> module as a whole?
> 
> Do you mean, rephrase it so it talks about what kind of thing the
> module contains, rather than what the class does.

Right.

> > I'm not sure about the design of this module.  Separating out the storage
> layer is obviously very useful, and it looks as if you could do with a
> setAllRules in NullFeatureRuleSource as well.  But the testing code and
> production-usable code live very close together here.  Wouldn't it be more
> appropriate (and consistent with our existing codebase) to have a functional
> FeatureRuleSource for production, plus a FakeRuleSource in lp.testing that
> overrides the two storage methods?
> 
> If the consistent pattern is that implementations only for testing use
> go into lp.testing then I'll move it there.

I think it is, yes.  A right little fakes library is forming there.  Before that, we usually put most such things in the individual tests.  (And, as you're probably about to say, repeated them a lot).

More to follow in part two!

« Back to merge proposal