Code review comment for lp:~jtv/launchpad/recife-552639

Here one comment, I forgot.

> === modified file 'lib/lp/translations/tests/test_translationmessage.py'
> --- lib/lp/translations/tests/test_translationmessage.py 2009-08-25 20:15:38 +0000
> +++ lib/lp/translations/tests/test_translationmessage.py 2010-06-19 03:38:37 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Unit tests for `TranslationMessage`."""
> @@ -10,12 +10,46 @@
> from zope.component import getUtility
> from zope.security.proxy import removeSecurityProxy
>
> +from canonical.launchpad.webapp.testing import verifyObject
> from lp.services.worlddata.interfaces.language import ILanguageSet
> from lp.testing import TestCaseWithFactory
> from lp.translations.model.potranslation import POTranslation
> +from lp.translations.model.translationmessage import DummyTranslationMessage
> +from lp.translations.interfaces.translationmessage import (
> + ITranslationMessage)
> from lp.translations.interfaces.translations import TranslationConstants
> from canonical.testing import ZopelessDatabaseLayer
>
> +
> +class TestTranslationMessage(TestCaseWithFactory):
> + """Unit tests for `TranslationMessage`.
> +
> + There aren't many of these. We didn't do much unit testing back
> + then.
> + """

And thanks a lot for adding these! ;-)

> +
> + layer = ZopelessDatabaseLayer
> +
> + def test_baseline(self):
> + message = self.factory.makeTranslationMessage()
> + verifyObject(ITranslationMessage, message)

I'd have split this test here.

> +
> + pofile = self.factory.makePOFile('nl')
> + potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
> + dummy = DummyTranslationMessage(pofile, potmsgset)
> + verifyObject(ITranslationMessage, dummy)
> +
> + def test_is_diverged(self):
> + # ITranslationMessage.is_diverged is a little helper to let you
> + # say "message.is_diverged" which can be clearer than
> + # "message.potemplate is not None."
> + message = self.factory.makeTranslationMessage(force_diverged=False)
> + self.assertFalse(message.is_diverged)
> +

And this one here.

> + message = self.factory.makeTranslationMessage(force_diverged=True)
> + self.assertTrue(message.is_diverged)

But that is just how I would do that. Makes test failures easier to spot.
Also, unrelated parts of a test will not be run if one fails. Maybe that
second reasoning is even stronger.

> +
> +
> class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
> """Tests for `TranslationMessage.findIdenticalMessage`."""
>

Cheers, Henning