Re: [PATCH 0/6] easy bulk commit creation in tests

From: Jeff King <peff@peff.net>
Date: Jun 29, 2019, 00:45 UTC
Message-ID: <20190629004559.GC3094@sigill.intra.peff.net>
In-Reply-To: <87v9wp7dfp.fsf@evledraar.gmail.com>

On Fri, Jun 28, 2019 at 08:49:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 12 quoted lines

> > So here's a patch to do that. Writing the bulk commit function was a fun
> > exercise, and I found a couple other places to apply it, too, shaving
> > off ~7.5 seconds from my test runs. Not ground-breaking, but I think
> > it's nice to have a solution where we don't have to be afraid to
> > generate a bunch of commits.
> 
> Nice.
> 
> Just a side-note: I've wondered how much we could speed up the tests in
> other places if rather than doing setup all over the place we simply
> created a few "template" repository shapes, and the common case for
> tests would be to simply cp(1) those over.

That thought also occurred to me while writing this. I've worked with test suites that have those kind of "fixtures" before, and I generally like it less, for two reasons:

  - it's much harder to understand what's important about the fixture,
    because you're seeing the end result of running a bunch of commands.
    Whereas the individual commands that show you _how_ it was derived
    are generally instructive; they give you the steps that the author
    was thinking about.

  - it's more annoying to update them because you don't just change the
    instructions. You have to extract the fixture into a real repo,
    manipulate it, then convert it back into whatever storage format we
    use (which can't just be a real repo, because we don't allow
    embedding repos).

But what I think _would_ be cool is to treat the command instructions as the source of truth, but allow caching of the on-disk state at certain points in a test script. I.e., imagine that we annotate some test snippets to say "I am setup, and it's OK if you don't run me every time". Like:

  test_expect_success SETUP 'a really slow setup step' '
	for i in $(test_seq 1000)
	do
		test_commit horribly-slow-$i
	done
  '

and then the test harness would recognize the SETUP prereq as magical, and:

  - look for t/cache/t1234.42.tar; if it exists, then replace the whole
    trash-dir state with it and skip the test

  - otherwise run the snippet and create t1234.42.tar for next time

Bonus points if you could specifically ask to cache t1234.57, even if it _isn't_ marked as SETUP, and then restart the script from that point, skipping over the intermediate tests (whether they were cached or not). That would let you then run subsequent tests from a known point instantly (e.g., if you're debugging some later test in the script and want to run it over and over).

The downsides I see are:

  1. It doesn't exercise the setup snippets as much. The idea is that
     this shouldn't matter if it's just setup code, but I'm sure we do
     get some extra coverage from it. But any fixture-based scheme
     suffers from this.

  2. Cache invalidation (isn't it always?). If you changed the setup
     test or even fixed a bug elsewhere in Git, you'd want to re-run the
     setup steps. It's always OK to blow away the cache and get a fresh
     run, but sometimes it's easy to forget to do so (and the results
     can be confusing).

-Peff