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