launchpad-dev team mailing list archive

[Bjorn Tillenius <bjorn@xxxxxxxxxxxxx>]

On Tue, Dec 08, 2009 at 01:51:16AM -0000, noreply@xxxxxxxxxxxxx wrote:
> Merge authors:
>   Edwin Grubbs (edwin-grubbs)
> ------------------------------------------------------------
> revno: 9986 [merge]
> committer: Launchpad Patch Queue Manager <launchpad@xxxxxxxxxxxxxxxxx>
> branch nick: launchpad
> timestamp: Tue 2009-12-08 01:48:50 +0000
> message:
>   [r=abentley][ui=none][bug=433809] Fixed ProductSeriesVocabulary
>   	searches containing a slash.
> added:
>   lib/lp/registry/tests/test_productseries_vocabularies.py
> modified:
>   lib/lp/registry/vocabularies.py
> 
> 
> --
> lp:launchpad/devel
> https://code.launchpad.net/~launchpad-pqm/launchpad/devel
> 
> You are subscribed to branch lp:launchpad/devel.
> To unsubscribe from this branch go to https://code.launchpad.net/~launchpad-pqm/launchpad/devel/+edit-subscription.

> === added file 'lib/lp/registry/tests/test_productseries_vocabularies.py'
> --- lib/lp/registry/tests/test_productseries_vocabularies.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/tests/test_productseries_vocabularies.py	2009-12-07 20:13:51 +0000
> @@ -0,0 +1,72 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test the milestone vocabularies."""
> +
> +__metaclass__ = type
> +
> +from unittest import TestLoader
> +from operator import attrgetter
> +
> +from lp.registry.vocabularies import ProductSeriesVocabulary
> +from lp.testing import TestCaseWithFactory
> +
> +from canonical.testing import DatabaseFunctionalLayer
> +
> +
> +class TestProductSeriesVocabulary(TestCaseWithFactory):
> +    """Test that the ProductSeriesVocabulary behaves as expected."""
> +    layer = DatabaseFunctionalLayer

Great that you added some better tests; the current vocabulary tests are
lacking quite a bit.


> +    def setUp(self):
> +        super(TestProductSeriesVocabulary, self).setUp()
> +        self.vocabulary = ProductSeriesVocabulary()
> +        self.product_prefix = 'asdf987-'
> +        self.series1_prefix = 'qwerty-'
> +        self.product = self.factory.makeProduct(
> +            self.product_prefix + 'product1')
> +        self.series = self.factory.makeProductSeries(
> +            product=self.product, name=self.series1_prefix + "series1")
> +
> +    def tearDown(self):
> +        super(TestProductSeriesVocabulary, self).tearDown()
> +
> +    def test_search(self):
> +        series2 = self.factory.makeProductSeries(product=self.product)
> +        # Search by product name.
> +        result = self.vocabulary.search(self.product.name)
> +        self.assertEqual(
> +            [self.series, series2].sort(key=attrgetter('id')),
> +            list(result).sort(key=attrgetter('id')))
> +        # Search by series name.
> +        result = self.vocabulary.search(self.series.name)
> +        self.assertEqual([self.series], list(result))
> +        # Search by series2 name.
> +        result = self.vocabulary.search(series2.name)
> +        self.assertEqual([series2], list(result))
> +        # Search by product & series name substrings.
> +        result = self.vocabulary.search(
> +            '%s/%s' % (self.product_prefix, self.series1_prefix))
> +        self.assertEqual([self.series], list(result))

Two comments. What value do the comments you added add? When commenting
in tests, I think it's better to think more in terms of doctests. What
happens if you remove all the code? Do the comments still make sense,
without the code?

Also, why is this one single big test, instead of multiple smaller ones?
I had a hard time finding the test that actually tested the issue you
claimed to fix (searches containing a slash), and I think it would have
been easier if there had been a specific test for that. Smaller tests
makes it easier to see what actually is the problem, if a test starts to
fail.


> +
> +    def test_toTerm(self):
> +        term = self.vocabulary.toTerm(self.series)
> +        self.assertEqual(
> +            '%s/%s' % (self.product.name, self.series.name),
> +            term.token)
> +        self.assertEqual(self.series, term.value)
> +
> +    def test_getTermByToken(self):
> +        token = '%s/%s' % (self.product.name, self.series.name)
> +        term = self.vocabulary.getTermByToken(token)
> +        self.assertEqual(token, term.token)
> +        self.assertEqual(self.series, term.value)
> +
> +    def test_getTermByToken_LookupError(self):
> +        self.assertRaises(
> +            LookupError,
> +            self.vocabulary.getTermByToken, 'does/notexist')

These all lack comments explaining the tests.



-- 
Björn Tillenius | https://launchpad.net/~bjornt