Merge lp:~jml/testresources/tests-meaning-cleanup into lp:~lifeless/testresources/trunk

As I don't have a mail to reply to, I've copied the lp page here and
marked replies with '*****'

Where I haven't said anything, its good in principle.

Once we're agreed conceptually I'd do an actual diff review.

----

Hey Rob,

I've done a *lot* of hacking over the weekend. I got into the zone a
bit,
which meant that I didn't really break the work up as well as I could
have.

The diff is big -- 1800+ lines -- so it might be easier for you to just
look
over the new codebase.

My changes are broken down below. This letter might be converted into a
NEWS
file some day.

 LICENSE | 19

Move the copyright statements from all over the code into a top-level
LICENSE
file. Almost every file was affected by the move, so I won't mention it
in
their summaries.

The license is still GPL v2 or better. Copyright is Robert Collins
2005-2008,
except for priodict and dijkstra. I've filled in information about those
last
two as best as I can without Internet.

***** Please don't do this, best practice for the GPL is the
header-per-file as I had, and as bzr (for example) does. The LICENCE
file also fails to reference COPYING which is the actual copyright
licence.

 lib/testresources/__init__.py | 333 ++++++++-------

A lot of stuff changed here, obviously.
- Renamed OptimisingTestSuite to OptimizingTestSuite.

**** Eep, now I see it, I'm more pro UK spelling than being ambivalent,
I can deal, but ugh.

- Got rid of all the classmethod stuff on TestResource. Now it works
much more
  like a normal Python object.
- Renamed ResourcedTestCase._resources to resources.
- Renamed _makeResource to makeResource.
- Renamed setResource to _setResource.

**** This seems inconsistent, why make that one method private when the
general thrust is making things more public?

- Renamed _cleanResource to cleanResource.
- Moved a bunch of stuff out of OptimizingTestSuite into top-level
functions.
  The goal here was to make things more testable.

**** I'll need to look at the details, unless they are reusable
fragments I'm not sure that this is better.

- Renamed adsorbSuite to flatAddTest.

**** Why? (if the intent is the same, but you want an addTest in the
name, I'd suffix addTests rather than prefixing, so in doc generation
its more visible).

I disabled the crazy intermittent test after having sussed out *why* it
was
intermittent (although not why *.pyc files had an effect).

**** expectFail please, rather than disable.

You'll also notice that I call instances of TestResource
'resource_managers'.
I couldn't think of anything better at the time.

**** That suggests ResourceManager rather than TestResource.

-Rob

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

On Thu, Aug 21, 2008 at 2:57 PM, Robert Collins
<email address hidden> wrote:
> As I don't have a mail to reply to, I've copied the lp page here and
> marked replies with '*****'
>
> Where I haven't said anything, its good in principle.
>
> Once we're agreed conceptually I'd do an actual diff review.
>
> ----
>
> Hey Rob,
>
> I've done a *lot* of hacking over the weekend. I got into the zone a
> bit,
> which meant that I didn't really break the work up as well as I could
> have.
>
> The diff is big -- 1800+ lines -- so it might be easier for you to just
> look
> over the new codebase.
>
> My changes are broken down below. This letter might be converted into a
> NEWS
> file some day.
>
>
> LICENSE | 19
>
> Move the copyright statements from all over the code into a top-level
> LICENSE
> file. Almost every file was affected by the move, so I won't mention it
> in
> their summaries.
>
> The license is still GPL v2 or better. Copyright is Robert Collins
> 2005-2008,
> except for priodict and dijkstra. I've filled in information about those
> last
> two as best as I can without Internet.
>
> ***** Please don't do this, best practice for the GPL is the
> header-per-file as I had, and as bzr (for example) does. The LICENCE
> file also fails to reference COPYING which is the actual copyright
> licence.
>

OK. I'll revert this change, and if necessary bring the code inline
with http://www.gnu.org/licenses/gpl-howto.html

>
>
> lib/testresources/__init__.py | 333 ++++++++-------
>
> A lot of stuff changed here, obviously.
> - Renamed OptimisingTestSuite to OptimizingTestSuite.
>
> **** Eep, now I see it, I'm more pro UK spelling than being ambivalent,
> I can deal, but ugh.
>

:)

> - Got rid of all the classmethod stuff on TestResource. Now it works
> much more
> like a normal Python object.
> - Renamed ResourcedTestCase._resources to resources.
> - Renamed _makeResource to makeResource.
> - Renamed setResource to _setResource.
>
> **** This seems inconsistent, why make that one method private when the
> general thrust is making things more public?
>

The thrust is making the bits that people are supposed to *use*
public, and making the bits that are just there to reduce code
duplication private.

As best as I could tell _setResource has no reason to be public. In
fact, I was tempted to remove it at one point.

> - Renamed _cleanResource to cleanResource.
> - Moved a bunch of stuff out of OptimizingTestSuite into top-level
> functions.
> The goal here was to make things more testable.
>
> **** I'll need to look at the details, unless they are reusable
> fragments I'm not sure that this is better.
>

The functions are:
- cost_of_switching(old_resource_set, new_resource_set)
- switch(old_resource_set, new_resource_set)
- split_by_resources(tests)

In this light, I think the first two should be made methods on
OptimizingTestSuite (I can imagine someone wanting to override them).
I can probably be persuaded to think the same thing about
split_by_resources, which splits 'tests' into 'tests that use
resources' and 'tests that don't'.

> - Renamed adsorbSuite to flatAddTest.
>
> **** Why? (if the intent is the same, but you want an addTest in the
> ...

On Thu, 2008-08-21 at 08:33 +0000, Jonathan Lange wrote:

> > LICENSE | 19
> >
> > Move the copyright statements from all over the code into a top-level
> > LICENSE
> > file. Almost every file was affected by the move, so I won't mention it
> > in
> > their summaries.
> >
> > The license is still GPL v2 or better. Copyright is Robert Collins
> > 2005-2008,
> > except for priodict and dijkstra. I've filled in information about those
> > last
> > two as best as I can without Internet.
> >
> > ***** Please don't do this, best practice for the GPL is the
> > header-per-file as I had, and as bzr (for example) does. The LICENCE
> > file also fails to reference COPYING which is the actual copyright
> > licence.
> >
>
> OK. I'll revert this change, and if necessary bring the code inline
> with http://www.gnu.org/licenses/gpl-howto.html

It should have been inline before; please just put it back the way it
was; I'd rather not have copyright changes that are dubious in the
history at all in fact - you probably need to branch from before these
changes and replay patches across.

I'd be happy to review a separate branch discussing copyright changes
though, but lets not conflate things.

-Rob

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

On Fri, Aug 22, 2008 at 10:45 AM, Robert Collins
<email address hidden> wrote:
> On Thu, 2008-08-21 at 08:33 +0000, Jonathan Lange wrote:
>
>> > LICENSE | 19
>> >
>> > Move the copyright statements from all over the code into a top-level
>> > LICENSE
>> > file. Almost every file was affected by the move, so I won't mention it
>> > in
>> > their summaries.
>> >
>> > The license is still GPL v2 or better. Copyright is Robert Collins
>> > 2005-2008,
>> > except for priodict and dijkstra. I've filled in information about those
>> > last
>> > two as best as I can without Internet.
>> >
>> > ***** Please don't do this, best practice for the GPL is the
>> > header-per-file as I had, and as bzr (for example) does. The LICENCE
>> > file also fails to reference COPYING which is the actual copyright
>> > licence.
>> >
>>
>> OK. I'll revert this change, and if necessary bring the code inline
>> with http://www.gnu.org/licenses/gpl-howto.html
>
> It should have been inline before; please just put it back the way it
> was; I'd rather not have copyright changes that are dubious in the
> history at all in fact - you probably need to branch from before these
> changes and replay patches across.
>
> I'd be happy to review a separate branch discussing copyright changes
> though, but lets not conflate things.
>

Sure. Will do.

jml

On Fri, Aug 22, 2008 at 10:45 AM, Robert Collins
<email address hidden> wrote:
> On Thu, 2008-08-21 at 08:33 +0000, Jonathan Lange wrote:
>
>> > LICENSE | 19
>> >
>> > Move the copyright statements from all over the code into a top-level
>> > LICENSE
>> > file. Almost every file was affected by the move, so I won't mention it
>> > in
>> > their summaries.
>> >
>> > The license is still GPL v2 or better. Copyright is Robert Collins
>> > 2005-2008,
>> > except for priodict and dijkstra. I've filled in information about those
>> > last
>> > two as best as I can without Internet.
>> >
>> > ***** Please don't do this, best practice for the GPL is the
>> > header-per-file as I had, and as bzr (for example) does. The LICENCE
>> > file also fails to reference COPYING which is the actual copyright
>> > licence.
>> >
>>
>> OK. I'll revert this change, and if necessary bring the code inline
>> with http://www.gnu.org/licenses/gpl-howto.html
>
> It should have been inline before; please just put it back the way it
> was; I'd rather not have copyright changes that are dubious in the
> history at all in fact - you probably need to branch from before these
> changes and replay patches across.
>
> I'd be happy to review a separate branch discussing copyright changes
> though, but lets not conflate things.

I've pushed up a new version that no longer has the license changes
and makes the changes I agreed to above.

I'm looking forward to hearing your reply.

jml

    * testresources now depends on pyunit3k. You can still use
testresources
      without using pyunit3k directly. (Jonathan Lange)

Thats not clear. Do you mean 'testresources needs pyunit3k to run its
own test suite'. Or 'if you use testresources you need to have pyunit3k
too.

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

One of the points of SampleTestResource was to demonstrate how to create
a resource for users of the library. We need to reintroduce that
demonstration. This could be a doc file, part of README, whatever.

With the change to instance methods, I think make() and clean() would be
better method names rather than makeResource() and cleanResource().

You've left the copyright munging change in README - it too needs
reverting.

+
+ XXX - that last sentence needs some unpacking, I think. -- jml
+

The sentence that needs unpacking- Its saying that if you add everything
to a single OTS, you get global optimisation. Or you could use several
smaller OTSs.

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])]

The refactoring to 'self.switch' has a bug: an exception raised during
the switch will no longer leave the right resources setup on the OTS
object.

"p" before "r", or a line of VWS to separate stdlib and local imports.

+import random
+import pyunit3k
 import testresources
-import testresources.tests
+from testresources import split_by_resources

There doesn't seem to be a test that the sortTests() method is invoked
anymore.

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.

The previous review was a conceptual one based on your mail, this was
based on the detailed code. With all of these points addressed I'll
merge it immediately.

-Rob

On Sat, Sep 6, 2008 at 5:27 PM, Robert Collins
<email address hidden> wrote:
>
> * testresources now depends on pyunit3k. You can still use
> testresources
> without using pyunit3k directly. (Jonathan Lange)
>
> Thats not clear. Do you mean 'testresources needs pyunit3k to run its
> own test suite'. Or 'if you use testresources you need to have pyunit3k
> too.
>

Rephrased to:
    * testresources needs pyunit3k to run the testresources test suite. You
      can still use testresources without using pyunit3k. (Jonathan Lange)

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

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

I've changed it back to OptimisingTestSuite. I still think that using
a non-US spelling will create unnecessary friction for new users: both
with Americans and with people for whom English is a second language.

I also went away to do some research. Here's what I've found:

    * The shorter OED lists "optimize" as the primary (and preferred) spelling.
    * The dictionary introduction states that Oxford University Press
prefers "-ize" to "-ise".
    * Fowler's has a list of words for which "-ise" is compulsory:
"optimise" is not included.
    * In the general entry on "-ize, -ise in verbs", Fowler's says,
"The matter remains delicately balanced but unresolved. The primary
rule is that all words of the type authorize/authorise,
civilize/civilise, legalize/legalise may legitimately be spelt with
either -ize or -ise throughout the English-speaking world except in
America, where -ize is compulsory."

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 :)

As I understand it, we have two operations for adding tests: addTest
and METHOD (aka addTestFlat). The first preserves the structure of the
added tests, specifically by including any test suite wrappers. The
second flattens all structure. Perhaps you intend "adsorbSuite" to be
something more general than that, and that's why we're having trouble
fixing on a name.

> One of the points of SampleTestResource was to demonstrate how to create
> a resource for users of the library. We need to reintroduce that
> demonstration. This could be a doc file, part of README, whatever.
>

I've added this to doc/example...

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.

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

> > One of the points of SampleTestResource was to demonstrate how to create
> > a resource for users of the library. We need to reintroduce that
> > demonstration. This could be a doc file, part of README, whatever.
> >
>
> I've added this to doc/example.py, and added a reference to it in the README.
>
> As an example, I think it could stand to be improved.

good examples > bad examples > no examples :)

> > +
> > + XXX - that last sentence needs some unpacking, I think. -- jml
> > +
> >
> > The sentence that needs unpacking- Its saying that if you add everything
> > to a single OTS, you get global optimisation. Or you could use several
> > smaller OTSs.
> >
>
> The two paragraphs now say:
>
> OptimisingTestSuite has a new method over normal TestSuites:
> addTestFlat(test_case_or_suite), which scans another test suite and
> incorporates all of its tests directly into the OptimisingTestSuite.
>
> Because the test suite does the optimisation, you can control the amount of
> optimising that takes place by adding more or fewer tests to a single
> OptimisingTestSuite. You could add everything to a single OptimisingTestSuite,
> getting global optimisation or you could use several smaller
> OptimisingTestSuites.

These words are good, given the current adding behaviour.

> > I think your changes sho...

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

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]))._te...

On Mon, Sep 8, 2008 at 11:48 AM, Robert Collins
<email address hidden> wrote:
> 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 ?
>

The second NEWS item is that if you do:
resource = resource_manager.getResource()
resource_manager.dirty(resource)
resource = resource_manager.getResource()

Then the second invocation of getResource will trigger a clean. This
is separate setting self._dirty on unused resources.

jml

On Mon, 2008-09-08 at 01:54 +0000, Jonathan Lange wrote:
>
> The second NEWS item is that if you do:
> resource = resource_manager.getResource()
> resource_manager.dirty(resource)
> resource = resource_manager.getResource()
>
> Then the second invocation of getResource will trigger a clean. This
> is separate setting self._dirty on unused resources.

Uhm. Something must have changed under the hood - some calling pattern
is different.

Certainly either what I had was broken, or I'd expect similar things to
preserve state appropriately as well.

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

On Mon, Sep 8, 2008 at 12:12 PM, Robert Collins
<email address hidden> wrote:
> On Mon, 2008-09-08 at 01:54 +0000, Jonathan Lange wrote:
>>
>> The second NEWS item is that if you do:
>> resource = resource_manager.getResource()
>> resource_manager.dirty(resource)
>> resource = resource_manager.getResource()
>>
>> Then the second invocation of getResource will trigger a clean. This
>> is separate setting self._dirty on unused resources.
>
> Uhm. Something must have changed under the hood - some calling pattern
> is different.
>
> Certainly either what I had was broken, or I'd expect similar things to
> preserve state appropriately as well.
>

I think the current trunk is broken: you'll notice that it doesn't
have any branch for 'if self._dirty'. Certainly, the 'getResource;
dirty; getResource' call pattern doesn't have any tests.

The test I added for this is 'testDirtyingResourceTriggersRemake'.
It's supplemented by testUsingTwiceMakesAndCleansTwice and
testDirtyingWhenUnused. If the diff doesn't make it clear, then we can
discuss it when we are renaming addTestFlat.

jml

addTestFlat is back to adsorbSuite, with the documented understanding that it'll be deprecated in favor of smarter addTest methods.

We've clarified the issue with tests.

Oh hey look I can vote on my own branch :)

> > Thats not clear. Do you mean 'testresources needs pyunit3k to run its
> > own test suite'. Or 'if you use testresources you need to have pyunit3k
> > too.
> >
>
> Rephrased to:
> * testresources needs pyunit3k to run the testresources test suite. You
> can still use testresources without using pyunit3k. (Jonathan Lange)

That doesn't appear to be true though. Right at the top of testresources/__init__.py there is "from pyunit3k import iterate_tests", so you need pyunit3k for a bit more than just running the test suite.

Good point!

IIRC, Rob's requirement was that people shouldn't have to subclass pyunit3k stuff. I'll push up a revision which doesn't have the import and see what he thinks.