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

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