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

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.py, and added a reference to it in the README.

As an example, I think it could stand to be improved.

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

Done.

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

Reverted.

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

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

I haven't made any changes in response to this item, pending clarification.

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

I'm struggling to sympathize with this one, given that
current_resources is a locally-scoped variable in both versions of the
code, and that the version in trunk doesn't handle exceptions at all.

Perhaps switch should be made private, or perhaps it should return the
new set of resources. The latter would allow subclasses to better
define their own error handling policies.

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

I just went for straight alpha-sorting.

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

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

> 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?
- 'bug' in switch
- name & purpose of addTestFlat / adsorbSuite particularly wrt addTest.

jml