Code review comment for lp:~jml/testresources/tests-meaning-cleanup

On Sun, 2008-09-07 at 23:03 +0000, Jonathan Lange wrote:
> On Mon, Sep 8, 2008 at 8:33 AM, Robert Collins
> <email address hidden> wrote:
> > On Sun, 2008-09-07 at 10:42 +0000, Jonathan Lange wrote:
> >
> >
> >> > * Calling finishedWith on a dirty resource now cleans that resource.
> >> > (Jonathan Lange)
> >> >
> >> > It already did that; I'm not sure why this is NEWS :). Specifically
> >> > though, it actually resets it if its dirty and still in use.
> >> >
> >>
> >> I see what happened here: I conflated two changes that were close
> >> together in the logs.
> >>
> >> I've added this entry to Internals:
> >> * If calling finishedWith on a TestResource reduces its usage count to
> >> zero, then the TestResource considers itself clean, i.e. _dirty is set
> >> to True. (Jonathan Lange)
> >>
> >> and have added this to a new bugfixes section:
> >> * Calling getResource on a dirty resource now triggers a clean and re-make
> >> of that resource. (Jonathan Lange)
> >>
> >> See revisions 56 and 57 for more information.
> >
> > This is the pending merge's diff:
> > - def finishedWith(cls, resource):
> > - cls._uses -= 1
> > - if cls._uses == 0:
> > - cls._cleanResource(resource)
> > - cls._currentResource = None
> > - elif cls._dirty:
> > - cls._cleanResource(resource)
> > - cls.setResource()
> > - finishedWith = classmethod(finishedWith)
> >
> >
> > So I don't see what's changed - the prior behaviour was:
> > if it is no longer in use it is cleaned
> > otherwise if it is dirty it is reset
> >
> > The new behaviour seems identical.
> >
>
> Note that the finishedWith above doesn't change the value of
> self._dirty when _uses is 0. In the new version, self._dirty is set to
> False if _uses is 0. That is all.

Ah yes. Ok, that certainly benefits by being documented. (Though the
dirty flag is irrelevant for an unused resource :)). That should be one
NEWS item though, not two ?

> >> > I don't really like addTestFlat - it doesn't encapsulate the different
> >> > behaviour enough. adsorbSuite did. Also please put Optimise back as the
> >> > spelling. Both are clear, but Optimise is better English.
> >> >
> >
> >> Regarding addTestFlat vs adsorbSuite, I don't see how absorbSuite
> >> better describes the different behaviour. If I did, I probably
> >> wouldn't have changed the name :)
> >
> > I am not enamoured of adsorbSuite; however addTestFlat is also not
> > really good - see the later discussion.
> >
>
> OK. Let's thrash this one out via voce.

> * We want to support adding other "special" test suites to
> OptimisingTestSuite. In particular, if we add a test suite that provides
> services to its tests to an OptimisingTestSuite, adsorbSuite / addTestFlat
> should not totally flatten the suite, but instead keep the suite, even if it
> changes the structure of the tests.
>
> e.g. addTest maintains the structure:
> >>> OptimisingTestSuite().addTest(SpecialSuite([a, b]))._tests
> [SpecialSuite([a, b])]
>
> Currently, addTestFlat destroys all suite structure:
> >>> OptimisingTestSuite().addTestFlat(SpecialSuite([a, b]))._tests
> [a, b]
>
> Instead, it should preserve the suite while changing the structure:
> >>> OptimisingTestSuite().addTestFlat(SpecialSuite([a, b]))._tests
> [SpecialSuite(a), SpecialSuite(b)]
>
> All of the tests in each of the resulting new SpecialSuites should have
> identical resource requirements so we can still optimise.
>
> It's long, but I think it gets the point across.

Yes.

Now for the voice call and we're good I think.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.