Re: [ANNOUNCE] git-test: run automated tests against a range of Git commits

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/10/2017 10:35 AM, Jeff King wrote:
> On Sun, Jan 08, 2017 at 10:52:25AM +0100, Michael Haggerty wrote:
>> [...]

Since my last email, I have implemented a bunch of what we discussed
[1]. Because of the new semantics, I also *renamed the main command* from

    git test range [...]

to

    git test run [...]

>> I'm thinking of maybe
>>
>> * If an argument matches `*..*`, pass it to `rev-list` (like now).

This was already implemented.

>> * Otherwise, treat each argument as a single commit/tree (i.e., pass it
>> to `rev-parse`).

This is now implemented, too. Plus, it is now allowed to specify
multiple commits and/or ranges in a single invocation.

>> * If no argument is specified, test `@{u}..` (assuming that an
>>   upstream is configured). Though actually, this won't be as
>>   convenient as it sounds, because (a) `git test` is often run
>>   in a separate worktree, and (2) on errors, it currently leaves the
>>   repository with a detached `HEAD`.

I decided that if no argument is specified, it makes more sense to test
HEAD if the working tree is clean, and the contents of the working tree
otherwise. In the latter case no results are stored to notes, but it is
still useful to be able to run a preconfigured test quickly while
working. These are both implemented.

>> * Support a `--stdin` option, to read a list of commits/trees to test
>>   from standard input. By this mechanism, users could use arbitrary
>>   `rev-list` commands to choose what to test.

This is now implemented, too.

> [...]
>> I think ideally `git notes add` would look for pre-existing notes, and:
>>
>> * If none are found, create an empty notes reference.
>>
>> * If pre-existing notes are found and there was no existing test with
>>   that name, probably just leave the old notes in place.
>>
>> * If pre-existing notes are found and there was already a test with
>>   that name but a different command, perhaps insist that the user
>>   decide explicitly whether to forget the old results or continue using
>>   them. This might help users avoid the mistake of re-using old results
>>   even if they change the manner of testing.
> 
> I'm not quite sure what you mean here. By "test" and "command", do you
> mean the test name that is used in the notes ref, and the command that
> it is defined as?

By "test", I mean a test as configured in `git-test` and referred to by
its name (e.g., in `--test=name`). Currently the only configuration for
a test is `test.<name>.command`, but I structured the namespace that way
to leave room to add more configuration in the future.

> In the notes-cache.c subsystem, the commit message stores a validity
> token which must match in order to use the cache. You could do something
> similar here (store the executed command in the commit message, and
> invalidate the cache the user has changed the command). The notes-cache
> stuff isn't available outside of the C code, though. You could either
> expose it, or just do something similar longhand.
> 
> Thinking about it, though, I did notice that the tree sha1 is not the
> only input to the cache. Things like config.mak (not to mention the
> system itself) contribute to the test results. So no system will ever be
> perfect, and it seems like just making an easy way to force a retest (or
> just invalidate the whole cache) would be sufficient.

It's true that the tree and the test command are not the only inputs to
the testing machinery. I wasn't hoping to build a watertight system to
prevent accidental use of old results when `config.mak` or something
else in the environment has changed. I was only trying to give the user
a reminder that if they change the command, it's a good time to consider
whether the old results have to be invalidated.

I suppose if we wanted to make this system more watertight, we could let
the test configuration specify the names of other files on which its
results depend; for example,

    test.default.command = make -j16 test
    test.default.auxiliaryInput = config.mak

and include some kind of hash of the auxiliary inputs in a validity
token. But that feels like overkill.

> [...]
>> Yeah, this is awkward, not only because many people don't know what to
>> make of detached HEAD, but also because it makes it awkward in general
>> to use `git test` in your main working directory. I didn't model this
>> behavior on `git rebase --interactive`'s `edit` command, because I
>> rarely use that. But I can see how they would fit together pretty well
>> for people who like that workflow.
> 
> Yeah, after sleeping on it, I think it's best if "git test" remains
> separate from that. It's primary function is to run the test, possibly
> serving up a cached answer. So it would be perfectly reasonable to do:
> 
>   git rebase -x 'git test range HEAD'
> 
> to accomplish the interactive testing (though perhaps just "git test"
> would be a nice synonym for that).

That invocation can now be written `git test run`, which is a bit
shorter. There could be a special case that `git test` is shorthand for
`git test run`, but it quickly gets ambiguous if the user wants to start
adding any `run` arguments.

> And then "jump to a thing that I know is broken" becomes a separate
> action, whether you are using "git test" or not. I wonder if we could
> have:
> 
>   git rebase -e HEAD~2
> 
> to do an interactive rebase, with "edit" for HEAD~2. I feel like
> somebody even proposed that at one point, but I don't think it got
> merged.

That sounds handy. (The GIT_EDITOR thing is pretty hacky.)

> And then "git test fix" basically becomes:
> 
>   git rebase -e "$(git test first-broken)"
> 
> though I think you'd still want a shorthand for that.

There could be a `git test fix --rebase <range>` that does this. Plus my
preference, `git test fix --branch=fix <range>`, which creates a branch
named `fix` at the first broken commit and checks it out. (If the fix is
nontrivial, my next step is usually to `git imerge rebase` the original
branch onto the `fix` branch.)

>>> I think it should be possible to script the next steps, though.
>>> Something like like "git test fix foo", which would: [...]
>>
>> I think you would usually only want to mark only the *first* broken
>> commit as "edit", because often errors cascade to descendant commits.
> 
> Yeah, I had a similar thought. OTOH, if you didn't say "--keep-going",
> you'd only have one breakage either way. [...]

True enough.

Thanks for all your feedback!

Michael

[1] https://github.com/mhagger/git-test