-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi Bryce, thank you for this nice refactoring. I really like this style when writing tests. I only have to small issues that I ask you to fix before landing this. Can you land yourself or do you need me to do that for you?
review approve
Cheers, Henning
Am 18.06.2011 06:47, schrieb Bryce Harrington: > === modified file 'lib/lp/bugs/browser/tests/test_bugtracker_component.py' > --- lib/lp/bugs/browser/tests/test_bugtracker_component.py 2011-06-03 05:47:32 +0000 > +++ lib/lp/bugs/browser/tests/test_bugtracker_component.py 2011-06-18 04:47:25 +0000 > @@ -40,13 +40,18 @@ > 'field.actions.save': 'Save', > } > > + def _makeComponent(self, name): > + return self.factory.makeBugTrackerComponent(name, self.comp_group) > + > + def _makeUbuntuSourcePackage(self, package_name): > + distro = getUtility(IDistributionSet).getByName('ubuntu') > + return self.factory.makeDistributionSourcePackage( > + sourcepackagename=package_name, distribution=distro) > + > def test_view_attributes(self): > - component = self.factory.makeBugTrackerComponent( > - u'Example', self.comp_group) > - distro = getUtility(IDistributionSet).getByName('ubuntu') > - package = self.factory.makeDistributionSourcePackage( > - sourcepackagename='example', distribution=distro) > - form = self._makeForm(package) > + component = self._makeComponent(u'Example') > + dsp = self._makeUbuntuSourcePackage('example') > + form = self._makeForm(dsp) > view = create_initialized_view( > component, name='+edit', form=form) > label = 'Link a distribution source package to Example component' > @@ -58,33 +63,38 @@ > self.assertEqual(url, view.cancel_url) > > def test_linking(self): > - component = self.factory.makeBugTrackerComponent( > - u'Example', self.comp_group) > - distro = getUtility(IDistributionSet).getByName('ubuntu') > - package = self.factory.makeDistributionSourcePackage( > - sourcepackagename='example', distribution=distro) > + component = self._makeComponent(u'Example') > + dsp = self._makeUbuntuSourcePackage('example') > + form = self._makeForm(dsp) > > self.assertIs(None, component.distro_source_package) > - form = self._makeForm(package) > + view = create_initialized_view( > + component, name='+edit', form=form) > + notifications = view.request.response.notifications
This line seems to be dead code, you can remove it.
> + self.assertEqual(dsp, component.distro_source_package) > + > + def test_linking_notifications(self): > + component = self._makeComponent(u'Example') > + dsp = self._makeUbuntuSourcePackage('example') > + form = self._makeForm(dsp) > + > view = create_initialized_view( > component, name='+edit', form=form) > self.assertEqual([], view.errors) > - > notifications = view.request.response.notifications > - self.assertEqual(component.distro_source_package, package) > - expected = ( > - u"alpha:Example is now linked to the example " > - "source package in ubuntu.") > - self.assertEqual(expected, notifications.pop().message) > + expected = """ > + alpha:Example is now linked to the example > + source package in ubuntu.""" > + self.assertTextMatchesExpressionIgnoreWhitespace( > + expected, notifications.pop().message) > > def test_unlinking(self): > - component = self.factory.makeBugTrackerComponent( > - u'Example', self.comp_group) > + component = self._makeComponent(u'Example') > distro = getUtility(IDistributionSet).getByName('ubuntu') > - dsp = self.factory.makeDistributionSourcePackage( > - sourcepackagename='example', distribution=distro) > + dsp = self._makeUbuntuSourcePackage('example') > component.distro_source_package = dsp > form = self._makeForm(None) > + > view = create_initialized_view( > component, name='+edit', form=form) > self.assertEqual([], view.errors) > @@ -94,31 +104,28 @@ > self.assertEqual(expected, notifications.pop().message) > > def test_cannot_doublelink_sourcepackages(self): > - # Two components try linking to same package > - component_a = self.factory.makeBugTrackerComponent( > - u'a', self.comp_group) > - component_b = self.factory.makeBugTrackerComponent( > - u'b', self.comp_group) > - distro = getUtility(IDistributionSet).getByName('ubuntu') > - package = self.factory.makeDistributionSourcePackage( > - sourcepackagename='example', distribution=distro) > - > - form = self._makeForm(package) > - view = create_initialized_view( > - component_a, name='+edit', form=form) > - notifications = view.request.response.notifications > - self.assertEqual([], view.errors) > - self.assertEqual(1, len(notifications)) > - self.assertEqual(package, component_a.distro_source_package) > - > - form = self._makeForm(package) > + ''' Two components try linking to same same package > + > + We must maintain a one-to-one relationship between components > + and source packages. However, users are bound to attempt to try > + to make multiple components linked to the same source package, > + so the view needs to be sure to not allow this to be done and > + pop up a friendly error message instead. > + '''
We commonly don't put docstrings on tests. If could just turn this into a comment, please.
> + component_a = self._makeComponent(u'a') > + component_b = self._makeComponent(u'b') > + package = self._makeUbuntuSourcePackage('example') > + form = self._makeForm(package) > + > + component_a.distro_source_package = package > view = create_initialized_view( > component_b, name='+edit', form=form) > self.assertIs(None, component_b.distro_source_package) > self.assertEqual([], view.errors) > notifications = view.request.response.notifications > self.assertEqual(1, len(notifications)) > - expected = ( > - "The example source package is already linked to " > - "alpha:a in ubuntu.") > - self.assertEqual(expected, notifications.pop().message) > + expected = """ > + The example source package is already linked to > + alpha:a in ubuntu.""" > + self.assertTextMatchesExpressionIgnoreWhitespace( > + expected, notifications.pop().message) > > === modified file 'lib/lp/bugs/doc/bugtracker.txt' OK -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk3/XHQACgkQBT3oW1L17ihBNgCgoPVgvP+NXNfvtnjKx0TnhnHI H2QAni0Nnnv150740yCHqoUcVphura+e =kuKg -----END PGP SIGNATURE-----
« Back to merge proposal
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Bryce,
thank you for this nice refactoring. I really like this style when writing
tests. I only have to small issues that I ask you to fix before landing this.
Can you land yourself or do you need me to do that for you?
review approve
Cheers,
Henning
Am 18.06.2011 06:47, schrieb Bryce Harrington: bugs/browser/ tests/test_ bugtracker_ component. py' bugs/browser/ tests/test_ bugtracker_ component. py 2011-06-03 05:47:32 +0000 bugs/browser/ tests/test_ bugtracker_ component. py 2011-06-18 04:47:25 +0000 actions. save': 'Save', self, name): makeBugTrackerC omponent( name, self.comp_group) cePackage( self, package_name): IDistributionSe t).getByName( 'ubuntu' ) makeDistributio nSourcePackage( me=package_ name, distribution= distro) attributes( self): makeBugTrackerC omponent( IDistributionSe t).getByName( 'ubuntu' ) makeDistributio nSourcePackage( me='example' , distribution= distro) package) nent(u' Example' ) uSourcePackage( 'example' ) initialized_ view( l(url, view.cancel_url) makeBugTrackerC omponent( IDistributionSe t).getByName( 'ubuntu' ) makeDistributio nSourcePackage( me='example' , distribution= distro) nent(u' Example' ) uSourcePackage( 'example' ) distro_ source_ package) package) initialized_ view( response. notifications
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -40,13 +40,18 @@
> 'field.
> }
>
> + def _makeComponent(
> + return self.factory.
> +
> + def _makeUbuntuSour
> + distro = getUtility(
> + return self.factory.
> + sourcepackagena
> +
> def test_view_
> - component = self.factory.
> - u'Example', self.comp_group)
> - distro = getUtility(
> - package = self.factory.
> - sourcepackagena
> - form = self._makeForm(
> + component = self._makeCompo
> + dsp = self._makeUbunt
> + form = self._makeForm(dsp)
> view = create_
> component, name='+edit', form=form)
> label = 'Link a distribution source package to Example component'
> @@ -58,33 +63,38 @@
> self.assertEqua
>
> def test_linking(self):
> - component = self.factory.
> - u'Example', self.comp_group)
> - distro = getUtility(
> - package = self.factory.
> - sourcepackagena
> + component = self._makeCompo
> + dsp = self._makeUbunt
> + form = self._makeForm(dsp)
>
> self.assertIs(None, component.
> - form = self._makeForm(
> + view = create_
> + component, name='+edit', form=form)
> + notifications = view.request.
This line seems to be dead code, you can remove it.
> + self.assertEqua l(dsp, component. distro_ source_ package) notifications( self): nent(u' Example' ) uSourcePackage( 'example' ) initialized_ view( l([], view.errors) response. notifications l(component. distro_ source_ package, package) l(expected, notifications. pop().message) MatchesExpressi onIgnoreWhitesp ace( pop().message) self): makeBugTrackerC omponent( nent(u' Example' ) IDistributionSe t).getByName( 'ubuntu' ) makeDistributio nSourcePackage( me='example' , distribution= distro) uSourcePackage( 'example' ) distro_ source_ package = dsp None) initialized_ view( l([], view.errors) l(expected, notifications. pop().message) doublelink_ sourcepackages( self): makeBugTrackerC omponent( makeBugTrackerC omponent( IDistributionSe t).getByName( 'ubuntu' ) makeDistributio nSourcePackage( me='example' , distribution= distro) package) initialized_ view( response. notifications l([], view.errors) l(package, component_ a.distro_ source_ package) package)
> +
> + def test_linking_
> + component = self._makeCompo
> + dsp = self._makeUbunt
> + form = self._makeForm(dsp)
> +
> view = create_
> component, name='+edit', form=form)
> self.assertEqua
> -
> notifications = view.request.
> - self.assertEqua
> - expected = (
> - u"alpha:Example is now linked to the example "
> - "source package in ubuntu.")
> - self.assertEqua
> + expected = """
> + alpha:Example is now linked to the example
> + source package in ubuntu."""
> + self.assertText
> + expected, notifications.
>
> def test_unlinking(
> - component = self.factory.
> - u'Example', self.comp_group)
> + component = self._makeCompo
> distro = getUtility(
> - dsp = self.factory.
> - sourcepackagena
> + dsp = self._makeUbunt
> component.
> form = self._makeForm(
> +
> view = create_
> component, name='+edit', form=form)
> self.assertEqua
> @@ -94,31 +104,28 @@
> self.assertEqua
>
> def test_cannot_
> - # Two components try linking to same package
> - component_a = self.factory.
> - u'a', self.comp_group)
> - component_b = self.factory.
> - u'b', self.comp_group)
> - distro = getUtility(
> - package = self.factory.
> - sourcepackagena
> -
> - form = self._makeForm(
> - view = create_
> - component_a, name='+edit', form=form)
> - notifications = view.request.
> - self.assertEqua
> - self.assertEqual(1, len(notifications))
> - self.assertEqua
> -
> - form = self._makeForm(
> + ''' Two components try linking to same same package
> +
> + We must maintain a one-to-one relationship between components
> + and source packages. However, users are bound to attempt to try
> + to make multiple components linked to the same source package,
> + so the view needs to be sure to not allow this to be done and
> + pop up a friendly error message instead.
> + '''
We commonly don't put docstrings on tests. If could just turn this into a
comment, please.
> + component_a = self._makeCompo nent(u' a') nent(u' b') uSourcePackage( 'example' ) package) a.distro_ source_ package = package initialized_ view( b.distro_ source_ package) l([], view.errors) response. notifications l(expected, notifications. pop().message) MatchesExpressi onIgnoreWhitesp ace( pop().message) bugs/doc/ bugtracker. txt' enigmail. mozdev. org/
/XHQACgkQBT3oW1 L17ihBNgCgoPVgv P+NXNfvtnjKx0Tn hnHI 40yCHqoUcVphura +e
> + component_b = self._makeCompo
> + package = self._makeUbunt
> + form = self._makeForm(
> +
> + component_
> view = create_
> component_b, name='+edit', form=form)
> self.assertIs(None, component_
> self.assertEqua
> notifications = view.request.
> self.assertEqual(1, len(notifications))
> - expected = (
> - "The example source package is already linked to "
> - "alpha:a in ubuntu.")
> - self.assertEqua
> + expected = """
> + The example source package is already linked to
> + alpha:a in ubuntu."""
> + self.assertText
> + expected, notifications.
>
> === modified file 'lib/lp/
OK
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAk3
H2QAni0Nnnv1507
=kuKg
-----END PGP SIGNATURE-----