Code review comment for lp://staging/~bryce/launchpad/bugtracker_component_test_refactor

Revision history for this message
Henning Eggers (henninge) wrote :

-----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-----

review: Approve

« Back to merge proposal