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

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.

My other change was to getResources:

Old code:
    def getResource(cls):
        if not hasattr(cls, "_uses"):
            cls._currentResource = None
            cls._dirty = False
            cls._uses = 0
        if cls._uses == 0:
            cls.setResource()
        cls._uses += 1
        return cls._currentResource
    getResource = classmethod(getResource)

New code:
    def getResource(self):
        if self._uses == 0:
            self._setResource(self.make())
        elif self._dirty:
            self._resetResource(self._currentResource)
        self._uses += 1
        return self._currentResource

Note that the new code has an 'if self._dirty' branch.

>> > 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.

>> > I think your changes should be (c) You, or alternatively, you can assign
>> > to me. I don't have an opinion as to whats better.
>> >
>> > Files you edit, you should assert (c) on by adding a line appropriately.
>> >
>> > +* It should be possible to make copies of TestSuites in adsorbSuite,
>> > keeping
>> > + the containing TestSuites but at a chosen granularity, so that all
>> > tests in
>> > + the resulting suites have identical resource requirements and
>> > allowing
>> > + optimisation to still occur.
>> > + XXX: Clarify with Robert.
>> >
>> > Say that you have a similar suite to OptimisingTestSuite, which provides
>> > a service to its contained tests. adsorbSuite should not flatten the
>> > tests but instead keep that suite. (Another reason addTestFlat doesn't
>> > fit that well as a name).
>> >
>> > Concrete example
>> >
>> > input = SpecialSuite(Test_WithResourceA, TestWithResourceB)
>> >
>> > optimisedSuite.METHOD(input)
>> >
>> > pp optimisedSuite._tests
>> > [SpecialSuite([Test_WithResourceA]),
>> > SpecialSuite([Test_WithResourceB])]
>> >
>>
>> This is covered by optimisedSuite.addTest(input). What should
>> adsorbSuite do here that addTest does not?
>
> addTest would not have split the suite up as my example tried to show.
>

Ahh, ok. Here's what the TODO item now says:

* 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.

>> > Why the move of test_suite from the top of
>> > lib/testresources/tests/test_FOO.py to the bottom? I consider it best
>> > practice to have test_suite (or load_tests) at the top of test scripts,
>> > to make them easy to find. Pls discuss.
>> >
>>
>> I personally found it clearer having them at the bottom, without
>> thinking too much on *why* they were clearer. Perhaps it's because
>> their content isn't particularly interesting.
>>
>> I'm not sure why test_suite (or load_tests) would need to be
>> particularly easy to find, as compared to anything else in the file.
>> In any case, the very bottom is almost as good as the very top for
>> finding things.
>>
>> I've left them at the bottom for now. Am happy to move them to the top
>> if you wish.
>
> Pep 8 doesn't provide any guidance. I do know that we do this pretty
> consistently throughout bzrlib; I'd like to do the same here.
>

Changed.

FWIW, we do it consistently at the bottom in Launchpad.

jml