Code review comment for lp:~bjornt/launchpad/windmill-test-layer

On Tue, Sep 15, 2009 at 02:29:24PM -0000, Brad Crittenden wrote:
> Review: Approve code
> Hi Bjorn,
>
> Thanks for taking on this testing infrastructure change. It'll make
> it a lot easier to run the windmill tests.
>
> > === modified file 'Makefile'
> > --- Makefile 2009-08-21 17:50:58 +0000
> +++ Makefile 2009-09-15 08:58:30 +0000
> > @@ -90,6 +90,8 @@
> > @echo
> > @echo "Running the JavaScript integration test suite"
> > @echo
> > + bin/test $(VERBOSITY) --layer=BugsWindmillLayer
> > + bin/test $(VERBOSITY) --layer=CodeWindmillLayer
> > bin/jstest
> >
> > check_mailman: build
>
> > === modified file 'lib/canonical/testing/layers.py'
> > --- lib/canonical/testing/layers.py 2009-08-21 19:38:21 +0000
> > +++ lib/canonical/testing/layers.py 2009-09-15 09:12:47 +0000
> > @@ -1638,3 +1643,53 @@
> > @profiled
> > def testTearDown(cls):
> > LayerProcessController.postTestInvariants()
> > +
> > +
> > +class BaseWindmillLayer(AppServerLayer):
> > + """Layer for Windmill tests.
> > +
> > + This layer shouldn't be used directly. A subclass needs to be
> > + created specifying which base URL to use (e.g.
> > + http://bugs.launchpad.dev:8085/).
> > + """
> > +
> > + base_url = None
> > + shell_objects = None
> > + config_file = None
> > +
> > + @classmethod
> > + @profiled
> > + def setUp(cls):
> > + if cls.base_url is None:
> > + # Only do the setup if we're in a subclass that defines
> > + # base_url. With no base_url, we can't create the config
> > + # file windmill needs.
> > + return
>
> Is the silent return sufficient or should there be an asssert?

It has to be a silent return. Due to the way layers work, this method
will be called on BaseWindmillLayer, before it's called on
BugsWindmillLayer. It felt like the easiest way of reducing code
duplicating between the different windmill layers.

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