test isolation: default ignores & repository acquisition

[Vincent Ladeuil]

A more complete answer requires code, which in the end, is the
only thing that matter. But the merge tests are not an easy
target, so I just comment below :-/

Roughly, the merge tests need love in at least 3 areas:
- more parametrization,
- make the merge *code* more testable,
- more support from MemoryTransport.

>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:

<snip/>

    jam> So I disagree with Vincent, which was my original
    jam> example. It isn't a big deal when there are 2 tests. But
    jam> when you have 2 pages of tests, tracking back to find
    jam> which test case this is part of is difficult.  Tracking
    jam> up 3 lines to see what setup is called is fairly
    jam> immediately obvious.

1) Shorter tests address your point, by making tests shorter you
can see more in the same page. That means: put more code in
setUp() and create higher level helpers.

2) I don't buy the argument that tests have to be on the same
page that their setup. It's a bit like saying that inheritance
can be used only if all classes fit in the same page.

    >> 
    >> Can you show me examples where your approach give benefits, and
    >> which ones[1] ?

    jam> So my direct set of test cases where this was an issue was:
    jam> bzrlib.tests.test_merge.py
    jam>   TestMergerEntriesLCA
    jam> and
    jam>   TestMergerEntriesLCAOnDisk

    jam> Now, all of those test cases have rather complex
    jam> setup. They are rather hard tests to decipher.

Right, that make them good candidates for refactoring. Complex
setup does not mean the test itself *has* to be hard to read (nor
write), it mostly means we lack the right setup tools.

    jam> They were just about as hard to create, and I tried to
    jam> do a decent job of explaining them in comments. (Not to
    jam> mention that the final "assertEqual" test is way to
    jam> complex, and the api is full of far too many
    jam> tuples... :)

All I can say at that point is that you recognize that the actual
tests are unclear. I agree, we should do better :)

    jam> I'm pretty sure that I could have used slightly more inheritance in some
    jam> of the tests. For example the:
    jam>   test_conflict_without_lca
    jam>   test_other_deletes_lca_renames
    jam>   test_executable_changes
    jam>   test_create_symlink
    jam>  ...

    jam> All use a similar shaped graph. And a couple of times the specific
    jam> difference is probably in just the final commit.

"just in the final commit" sounds to me as:
- common code in setUp()
- final commit in the test

    jam> Anyway, it was a pain for me to have to care whether I
    jam> had a disk backing to work with or not.

    jam> And figure out what tests could be done "cheaply" and
    jam> what tests could not (for comparison, first runs 23
    jam> cases in 2.4s = 104ms/test, OnDisk is 19 tests in 6.0s =
    jam> 315ms/test)

But only because you tried (and rightly so !) to separate tests
that could run without disk support from those that required disk
support.

So here I'm tempted to say that you may not have to care if:
1) we could use MemoryTransport in more cases,
2) the merge code was more testable.

    jam> Also note that there is a TestMergerInMemory() which has a similar split
    jam> out for things that I could test without creating a WT.

    jam> And note that TestMergerEntriesLCAOnDisk can't inherit from the same
    jam> hierarchy as TestMergerEntriesLCA because it needs the WithTransport
    jam> base class. (Arguably the whole thing could be pulled out to the side as
    jam> mixins that don't get their final base until the very end.)

Again, you may not need all of that if you can come closer to the code to test.

    jam> Now, potentially we could have turned all of these tests
    jam> into individual classes, and moved the setup
    jam> appropriately.

You generally find some commonality to group test by classes
(even when doing "test after" instead of "test before").

    jam> Or made a *much* bigger setUp that would have to deal
    jam> with 20 different permutations and then had the final
    jam> assertion test just test one aspect of that large set
    jam> up.

Different permutations is what parametrization is for.

    jam> I don't have great answers here. And if you can show me
    jam> a way to make those tests easier to
    jam> read/maintain/develop I'd be very happy to see it.

I certainly want to help here and I'll see what I can do.

    jam> I certainly created the "builder.build_snapshot()"
    jam> functionality explicitly to help me do the work, because
    jam> managing 'tree' objects and knowing what revision I was
    jam> on during 'tree.merge_from_branch' was killing me.

    jam> As is, there is a lot of voodoo there.

My gut feeling is that this is mainly due to the fact the code is
not testable enough.

    jam> Though I'm also pleased that I actually did do TDD as
    jam> much as possible there, and didn't fix things I knew
    jam> were wrong until I could get a test to exercise that
    jam> code.

Yup, I saw that on several occasions.

     Vincent