Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Feb 07 2019, Jeff King wrote:

> On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >> So far we've had the convention that these GIT_TEST_* variables,
>> >> e.g. the one for the commit graph, work the same way. Thus we guarantee
>> >> that we get (in theory) 100% coverage even when running the tests in
>> >> this special mode. I think it's better to keep it as-is.
>> >
>> > But what's the point of that? Don't you always have to run the test
>> > suite _twice_, once with the special variable and once without?
>> > Otherwise, you are not testing one case or the other.
>> >
>> > Or are you arguing that one might set many special variables in one go
>> > (to prefer running the suite only twice, instead of 2^N times). In which
>> > case we are better off running the test (as opposed to skipping it), as
>> > it might use one of the _other_ special variables besides
>> > GIT_TEST_PROTOCOL_VERSION.
>> >
>> > I can buy that line of reasoning. It still doesn't cover all cases that
>> > a true 2^N test would, but that clearly isn't going to be practical.
>>
>> Maybe I'm misunderstanding what you're proposing, but as an example,
>> let's say the test suite is just these two tests:
>>
>>     test_expect_success 'some unrelated thing' '...'
>>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'
>>
>> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test
>> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,
>>
>> I'd still like both tests to be run, not just 1/2 with
>> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly
>> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a
>> GIT_TEST_PROTOCOL_VERSION=1.
>
> But that's my "why". The second test will run identically in both runs,
> regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's
> value if you're only running the suite once in getting full coverage,
> but if you are literally going to run it with and without, then you're
> running the exact same code twice for no reason. And you have to run it
> both with and without, since otherwise all of the _other_ tests aren't
> seeing both options.

Yeah, by always running the 2nd test regardless of what
GIT_TEST_PROTOCOL_VERSION=* is set to we're wasting CPU if we know we're
going to run both with GIT_TEST_PROTOCOL_VERSION=1 and
GIT_TEST_PROTOCOL_VERSION=2.

But we don't know that, and in terms of CPU & time the tests that rely
on any given GIT_TEST_* flag are such a tiny part of the test suite,
that I think it's fine to run them twice in such a scenario to guard
against the case when we just run in one more or the other, and not
both.

>> IOW the point of these tests is to piggy-back on the tests that *aren't*
>> aware of the feature you're trying to test. So
>> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the
>> commit graph, and *also* those tests that are explicitly aware of the
>> commit graph, including those that for some reason would want to test
>> for the case where it isn't enabled (to e.g. test that --contains works
>> without the graph).
>>
>> Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than
>> not", and run just one test run with that, because I'll have blind spots
>> in the commit graph tests themselves, and would then need to do another
>> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.
>
> So if we are still talking about the same 2-test setup above, which only
> has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of
> being able to run a "stock" test, and then one with _both_ of the flags
> (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't
> quite know how each test will interact with all of the "other" flags.

Yeah that's another reason not to skip them, although we could imagine a
prereq where we skip the v2 tests if GIT_TEST_PROTOCOL_VERSION=1 is set
*and* no other GIT_TEST_*=* flag is set, but I think that would also be
a bad idea.

> So now I'm not 100% sure I understand what you're talking about, but I
> think maybe we are actually in agreement. ;)

I think there's two ways to view these GIT_TEST_FOO=BAR facilities:

 1. Run all tests with "FOO" set to "BAR", skip those (via prereq) we
    know would break in that mode.

 2. Ditto, but if a test says "I'm a test for FOO=!BAR" leave it alone.

#2 is what we have now. I read your
<20190206213458.GC12737@sigill.intra.peff.net> as suggesting that we
should move to #1.

You're correct that moving to #1 would force us to more exhaustively
mark things so that if the default moves to "BAR" we just have to flip a
switch.