Merge lp://staging/~jml/testtools/extract-factory into lp://staging/~testtools-committers/testtools/trunk

Does the factory need to be exposed at all?

On 15/12/2011 10:19 PM, "Jonathan Lange" <email address hidden> wrote:

Jonathan Lange has proposed merging lp:~jml/testtools/extract-factory into
lp:testtools.

Requested ...
Your team testtools developers is subscribed to branch lp:testtools.

=== modified file 'NEWS'
--- NEWS 2011-12-05 15:33:37 +0000
+++ NEWS 2011-12-15 11:18:22 +0000
@@ -14,6 +14,14 @@
  ``MatchesAll`` with keyword arguments, then this change might affect your
  test results. (Jonathan Lange)

+* ``TestCase`` now has a ``factory`` attribute, set to an instance of
+ ``ObjectFactory``. It now uses this instance for generating unique
strings
+ and integers. (Jonathan Lange)
+
+* ``TestCase.getUniqueInteger`` and ``TestCase.getUniqueString`` are now
+ deprecated. Use ``TestCase.factory.getUniqueInteger`` and
+ ``TestCase.factory.getUniqueString`` instead. (Jonathan Lange)
+
 Improvements
 ------------

=== modified file 'doc/for-test-authors.rst'
--- doc/for-test-authors.rst 2011-12-07 11:32:45 +0000
+++ doc/for-test-authors.rst 2011-12-15 11:18:22 +0000
@@ -1240,17 +1240,19 @@
 fine. However, sometimes it's useful to be able to create arbitrary
objects
 at will, without having to make up silly sample data.

-To help with this, ``testtools.TestCase`` implements creation methods
called
-``getUniqueString`` and ``getUniqueInteger``. They return strings and
-integers that are unique within the context of the test that can be used to
-assemble more complex objects. Here's a basic example where
-``getUniqueString`` is used instead of saying "foo" or "bar" or whatever::
+To help with this, ``testtools.TestCase`` comes with an ``ObjectFactory``
that
+you can access as the ``factory`` attribute within your test
+case. ``ObjectFactory`` implements creation methods called
``getUniqueString``
+and ``getUniqueInteger``. They return strings and integers that are unique
+within the context of the test that can be used to assemble more complex
+objects. Here's a basic example where ``getUniqueString`` is used instead
of
+saying "foo" or "bar" or whatever::

  class SomeTest(TestCase):

      def test_full_name(self):
- first_name = self.getUniqueString()
- last_name = self.getUniqueString()
+ first_name = self.factory.getUniqueString()
+ last_name = self.factory.getUniqueString()
          p = Person(first_name, last_name)
          self.assertEqual(p.full_name, "%s %s" % (first_name, last_name))

@@ -1260,13 +1262,15 @@
  class TestCoupleLogic(TestCase):

      def make_arbitrary_person(self):
- return Person(self.getUniqueString(), self.getUniqueString())
+ return Person(
+ self.factory.getUniqueString(),
+ self.factory.getUniqueString())

      def test_get_invitation(self):
          a = self.make_arbitrary_person()
          b = self.make_arbitrary_person()
          couple = Couple(a, b)
- event_name = self.getUniqueString()
+ event_name = self.factory.getUniqueString()
          invitation = couple.get_invitation(event_name)
          self.assertEqual(
              invitation,

=== modified file 'testtools/__init_...

On Thu, Dec 15, 2011 at 1:13 PM, Robert Collins
<email address hidden> wrote:
> Does the factory need to be exposed at all?

As in, does it need to be in the public API?

Not really, but it would allow Launchpad to delete a little bit of
code from its ObjectFactory, and make it easier for other projects to
build their own domain-specific factories.

jml

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.

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.

So the .factory is the only thing that really needs fixing, I'll mark this approved if you change factory to __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

So I think this became good to merge but we need to port it to git.