launchpad-dev team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

On Tue, Dec 8, 2009 at 1:37 AM, Bjorn Tillenius <bjorn@xxxxxxxxxxxxx> wrote:
> 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.


This was just the way I wrote the tests from the beginning, and the
benefits of rewriting seemed fairly small. I can definitely change it.


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

I can add that.

-Edwin