Re: [PATCH 4/4] t5326: test propagating hashcache values

On Tue, Sep 07 2021, Taylor Blau wrote:

Just in an effort to make myself clear here, because between your note in https://lore.kernel.org/git/YUOds4kcHRgMk5nC@nand.local/ and re-reading my mail here I can barely understand what I meant here :)

What I mean is that the whole "rm -rf" dance at the start of tests is fallout from an earlier call you made in c51f5a6437c (t5326: test multi-pack bitmap behavior, 2021-08-31) to split up three tests that really are the same logical test in terms of setup/teardown.

If they were to be re-combined as shown in the diff-on-top at the end here none of the tests need a "rm -rf repo" at the beginning, because they can all rely on a starting pattern of:

    test_when_finished "rm -rf repo" &&
    git init repo &&
    ( cd repo ... )

Or, you can also just not re-use the "repo" name, which is what I did in the fsck tests at https://lore.kernel.org/git/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@gmail.com/; then you don't even need test_when_finished "rm -rf[ ...]".

None of this requires fixing here or a re-roll, unless you happen to think this diff is the best thing since sliced bread (assume my Signed-off-by).

But just as a general musing on patterns to use in tests, I see why you used that 1x test as 3x, because you want "test_expect_success" to give you the label on for each "step".

I think that would be worth fixing in test-lib.sh, there's no inherent reason we couldn't support "checkpoints" or "subtests" within "test_expect_success" (the latter being a part of the TAP protocol).

But until then IMNSHO this sort of thing is an anti-pattern, i.e. needing to write things like:

    test_expect_success '[...]' 'A'
    test_expect_success '[...]' 'B'

Where a part of what B needs to do is to clean up the mess left behind A, or relies on its setup.

That's just added verbosity and context when reading the code. Ideally you'd just need to read the "setup" at the start, and then individual tests, with this pattern you need to read the preceding test(s) to see where the crap they're cleaning came from, and if it's even needed etc.

(For the "setup" part I've suggested a test_expect_setup, see https://lore.kernel.org/git/8735vrvg39.fsf@evledraar.gmail.com/).

The "needs the setup" part of this has the negative side-effect of breaking the semantics of the --run option. As noted in the E-Mail linked in the last paragraph it doesn't work well in general because of things like this, but let's steer in the right direction.

I.e. with this change you can run:

    ./t5326-multi-pack-bitmaps.sh --run=111-113

Which is great, usually you need at least 1,<nums you want> ,r 1-10,<nums you want> to get the setup. But because you split them this breaks:

    ./t5326-multi-pack-bitmaps.sh --run=112-113

I.e. we'll run only the 2nd and 3rd test in a sequence that needs the 1st for setup.

For context: My preference for this isn't just an asthetic or theoretical thing. It's really nice to be hacking some code, find that I've broken the bitmap code somehow in your test #112, and be able to just run (since running the whole thing takes 13s on my box, and I'd like to test in a tight loop):

    ./t5326-multi-pack-bitmaps.sh --run=112

But that breaks as noted above, so I need read earlier tests (I was probably looking at test #112 already at this point) and see how their setup works etc.

Anyway, as with so many things in git.git being able to just assume --run works like this isn't something you can rely on in the general case, but just as a matter of where we should be headed...

Yes, you're 100% right here. This was purely a misreading on my part, I managed to somehow not see/take into account the subshell. Using test_config makes no sense here.

The diff-on-top for discussion mentioned above: