testresources

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

tests-meaning-cleanup
Merge into trunk

Revision history for this message

Jonathan Lange (jml) wrote on 2008-08-21:

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
> name, I'd suffix addTests rather than prefixing, so in doc generation
> its more visible).
>

I'll suffix then.

The intent is manifold:
- I had to look up adbsorb in a dictionary — it looked like a spelling mistake.
- Even after knowing the definition, it didn't strongly communicate
the idea of losing nested structure.

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

We don't yet have that feature in pyunit3k :)

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

Or TestResourceManager. The thing is, a TestResource doesn't really
represent a single resource, it represents the ability to acquire and
relinquish a kind of resource.

> -Rob
>

Thanks.

Will ping you when I've made the changes I discussed earlier.

jml

On Thu, Aug 21, 2008 at 2:57 PM, Robert Collins
<robertc@robertcollins.net> 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)

I'll suffix then.

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

We don't yet have that feature in pyunit3k :)

Or TestResourceManager. The thing is, a TestResource doesn't really
represent a single resource, it represents the ability to acquire and
relinquish a kind of resource.

> -Rob
>

Thanks.

Will ping you when I've made the changes I discussed earlier.

jml

« Back to merge proposal