launchpad-reviewers team mailing list archive

[Thomi Richards <thomi.richards@xxxxxxxxxxxxx>]

Review: Needs Information

I'm really not happy about the mixin-containing-tests thing (see diff comment). However, if someone else (William?) thinks it's fine, then I'm happy to be overridden. I've seen too much code that relies on this idiom recently and has grown horribly complex as a result.

The rest looks fine though.

Diff comments:

> 
> === modified file 'lib/lp/snappy/browser/widgets/tests/test_snaparchivewidget.py'
> --- lib/lp/snappy/browser/widgets/tests/test_snaparchivewidget.py	2015-09-21 09:00:38 +0000
> +++ lib/lp/snappy/browser/widgets/tests/test_snaparchivewidget.py	2016-06-29 16:46:40 +0000
> @@ -29,16 +34,17 @@
>  from lp.testing.layers import DatabaseFunctionalLayer
>  
>  
> -class TestSnapArchiveWidget(TestCaseWithFactory):
> +class TestSnapArchiveWidgetMixin:

IMO, using a mixin that contains tests is a testing anti-pattern. It makes it hard to see what's being tested, and is ripe for abuse over time. In this case it's reasonably well contained, but if at all possible I think we should avoid this style of testing.

This is where testscenarios works really well. What do you think about adding testscenarios to launchpad, and using it here, and in other similar places as they occur?

>  
>      layer = DatabaseFunctionalLayer
>  
>      def setUp(self):
> -        super(TestSnapArchiveWidget, self).setUp()
> +        super(TestSnapArchiveWidgetMixin, self).setUp()
>          self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
> +        self.distroseries = self.factory.makeDistroSeries()
>          field = Reference(
>              __name__="archive", schema=IArchive, title=u"Archive")
> -        self.context = self.factory.makeSnap()
> +        self.context = self.makeContext(self.distroseries)
>          field = field.bind(self.context)
>          request = LaunchpadTestRequest()
>          self.widget = SnapArchiveWidget(field, request)


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-auto-build-archive-widget/+merge/297960
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-auto-build-archive-widget into lp:launchpad.