Code review comment for lp:~jml/testtools/extract-factory

On Wed, May 2, 2012 at 4:56 AM, Robert Collins
<email address hidden> wrote:
> Review: Approve
>
> I meant 'does the factory need to be a test case attribute'. To me, the factory is a multipurpose fixture, and fixtures are very usable (e.g. via scenarios) without being exposed on the testcase by default.
>
> >From the point of view of compat with prior testtools releases we can't drop our additional methods, but we can avoid adding an overly specific factory instance on every TestCase object. So putting it on e.g. __factory. That would let the new factory be used by other testing styles (e.g. nose), or evolved separately to TestCase.
>

OK. Thanks for clarifying.

FWIW, the price of this is making the factory slightly less convenient
to start using.

e.g. from

  x = self.factory.getUniqueString()

To

  factory = ObjectFactory()
  x = factory.getUniqueString()

To perhaps even

  def setUp(self):
    super(MyTestCase, self).setUp()
    self.factory = factory

  def test_something(self):
    x = self.factory.getUniqueString()

The structural correctness is probably worth this cost for now.

> Beyond that there is an interesting discussion about whether global factories are a good idea, or whether per test state is a better idea, and a little orthogonally, whether the factory should be a Fixture (which can then provide details etc) or an adhoc API.

I'd be interested in having that discussion ... later :)

>
> So the .factory is the only thing that really needs fixing, I'll mark this approved if you change factory to __factory.

I really don't like double underscores. But sure, let's get this
landed. Changing to drop one is easy and very low risk.

jml