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

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

> > 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.
> >
>
> Well, I did add something to the TODO saying we should test what
> happens when they raise an exception :)

Ah, I misread my old code. I thought it was using instance variables and
thus semi-atomic. Ignore this point.

> > There doesn't seem to be a test that the sortTests() method is invoked
> > anymore.
> >
>
> I didn't think it was necessary. As I saw it, sortTests isn't really
> part of the public interface and testResourceReuse already checks that
> the tests are optimized. Turns out that the docstring declares it to
> be a public hook.
>
> I've reinstated the test.

Thanks.

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

> > 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.
> >
>
> Cool. I look forward to your reply.
>
> Thanks for the review!
>
> Summary of outstanding issues:
> - move test_suite to the top or bottom?
    please put them back :)
> - 'bug' in switch
    done
> - name & purpose of addTestFlat / adsorbSuite particularly wrt addTest.
    stare harder at my example of what would be nice; then lets use
voice to sort it out.

-Rob

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